feat(traffictile.h): Adds versioning checks #2484
Conversation
src/baldr/CMakeLists.txt
Outdated
| list(APPEND sources | ||
| ${CMAKE_CURRENT_BINARY_DIR}/traffic_tile_version.h |
There was a problem hiding this comment.
should we use GENERATED_VERSION_HEADER here as well?
purew
force-pushed
the
version-traffic-tile-format
branch
from
88fb984 to
f790796
Compare
|
The |
Allocates one of the spare fields in the TrafficTileHeader for versioning data, with the version being made up of the first four bytes of the MD5 checksum of baldr/traffictile.h
purew
force-pushed
the
version-traffic-tile-format
branch
from
f790796 to
d60dac1
Compare
purew
force-pushed
the
version-traffic-tile-format
branch
from
1898c94 to
9c23054
Compare
valhalla/baldr/traffictile.h
Outdated
| if (header == nullptr) | ||
| if (header == nullptr) { | ||
| return INVALID_SPEED; | ||
| } else if (header->traffic_tile_version != TRAFFIC_TILE_VERSION) { |
There was a problem hiding this comment.
Why not have one if block and use an ||
There was a problem hiding this comment.
Logged different messages while debugging, I just pushed a commit that collaps them 👍
purew
force-pushed
the
version-traffic-tile-format
branch
from
385ea98 to
39b0fdd
Compare
|
Ok, I finally got all the platforms in CI passing after fixing the nvm setup in Osx and removing the pipes which upset Windows. cc @mandeepsandhu |
| echo 'nvm install v8.11.2' >> $BASH_ENV | ||
| echo 'nvm alias default v8.11.2' >> $BASH_ENV | ||
| source $BASH_ENV && nvm install v8.11.2 | ||
| source $BASH_ENV && nvm alias default v8.11.2 |
There was a problem hiding this comment.
CircleCi is supposed to source this itself, but on Osx it doesn't.
| # Builds a checksum of the traffictile header and store it in `traffic_tile_version.h` | ||
| set(GENERATED_VERSION_HEADER "${CMAKE_CURRENT_BINARY_DIR}/traffic_tile_version.h") | ||
| message("Generating traffictile version header") | ||
| configure_file(${VALHALLA_SOURCE_DIR}/valhalla/baldr/traffictile.h |
There was a problem hiding this comment.
@danpat suggested a way to evaluate this entirely in cmake, with the hack here to implicitly add a dependency on the file to re-run checksum on changes. My first attempt without this hack only generated the checksum on first run.
.circleci/config.yml
Outdated
| @@ -10,7 +10,7 @@ executors: | |||
| commands: | |||
| install_macos_dependencies: | |||
| steps: | |||
| - run: brew install protobuf cmake ccache libtool boost-python libspatialite pkg-config luajit curl wget czmq lz4 spatialite-tools unzip | |||
| - run: brew install protobuf cmake ccache coreutils libtool boost-python libspatialite pkg-config luajit curl wget czmq lz4 spatialite-tools unzip | |||
There was a problem hiding this comment.
This change isn't needed now that the checksum is done in pure cmake
There was a problem hiding this comment.
I left it in since it was listed as a build requirement on osx in readme: https://github.com/valhalla/valhalla#building-from-source
But I can remove it, fix pushed.
* 'master' of github.com:valhalla/valhalla: Make tile building reproducible (#2480) Fix install-script for ubuntu 18.04 (#2306) (#2486) nit(git): Configures changelog to resolve conflicts with union strategy (#2489) feat(traffictile.h): Adds versioning checks (#2484) Fix dereferencing of end for std::lower_bound and possible UB (#2488) Minor fixes to tests (#2483) nit: Missing changelog entry (#2478) Safiefy protobuf (#2477)
Allocates one of the spare fields in the TrafficTileHeader
for versioning data, with the version being made up of
the first four bytes of the MD5 checksum of baldr/traffictile.h