-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bins change on rebuild, even though there's no change in source code? #1229
Comments
Strange, indeed. Will investigate. |
Confirmed, it does indeed include a compile timestamp in the binary. |
hm, is that part of the cmake machinery? or can it be removed? |
There's code that picking up the compile time from the compiler through a macro: SimpleLogger().Write() << "starting up engines, " << g_GIT_DESCRIPTION << ", "
<< "compiled at " << __DATE__ << ", " __TIME__; |
is something similar used in extract and prepare? |
i guess. could we remove it? |
I would have to check, but I think yes. Checking right now if removing fixes things. |
ok |
Still some difference. will continue digging. |
Should we remove this from all binaries? |
yes if possible. if we can avoid change in bins, it can speed up tests a lot. for example, if you only change/relink osrm-routed, you don't need to extract and prepare data when rerunning tests, which will probably speed up tests 5-10 times. |
Progress: $ md5sum osrm-routed
25ec47bae4c9659d00f7889512654934 osrm-routed
$ make -j3 osrm-routed
[ 5%] Built target LOGGER
[ 11%] Built target COORDINATE
[ 17%] Configuring FingerPrint.cpp
[ 23%] Building CXX object CMakeFiles/OSRM.dir/Util/FingerPrint.cpp.o
Linking CXX static library libOSRM.a
[ 76%] Built target OSRM
Linking CXX executable osrm-routed
[100%] Built target osrm-routed
$ md5sum osrm-routed
25ec47bae4c9659d00f7889512654934 osrm-routed |
cool |
fix upcoming in the next minutes once local test run succeeds. |
nice |
if i rebuild without code change, then bins now stay the same. so far so good.
|
also worth noting is that a git commit will cause all bins to change because they include the git rev as part of the version info, all thus all caching will be invalidated except the basic osm file. |
The change in source must change the generated binary. For example, added or removed white spaces dont change the resulting binary. |
fair point, but the thing is that changing prepare.cpp causes all bins to change. why is this? is it due to the cmake fingerprint? it seems it combines md5s from multiple files: set(OLDFILE ${SOURCE_DIR}/Util/FingerPrint.cpp) CONFIGURE_FILE( ${SOURCE_DIR}/Util/FingerPrint.cpp.in ${SOURCE_DIR}/Util/FingerPrint.cpp ) |
actually changing whitespace does result in a recompile and a change in alls bin, as can be seen from the log above. |
Yes, it takes the md5sum of prepare.cpp thus changing the resulting checksums.
right, but this is only true for our build. We want the checksum to change when any file that contains I/O code changes. |
I meant to say: We want the fingerprint to change when any file that contains I/O code changes. |
i'm not sure i understand the cmake fingerprinting. |
is the fingerprint used when saving files? |
It is used to detect when data files have been prepared with a different build, i.e. git revision. Then a warning is issued. |
for releases, we want to make sure all the bins use the same fingerprint, or we would get warnings about mismatched fingerprints. this is what happens now; a change in prepare.cpp causes all bins to be rebuild, with the same new fingerprint. during development we don't want a change in fingerprint to force a rebuild of all bins, because we want to be able to change source code for one bin without causing all bins to be rebuild (and cache files to be invalidated). this implies that the different bins might use different fingerprints during tests, and that we would have to handle that somehow. |
the fingerprint is not linked to git revision, but rather to md5 hashes of key source code files, right? |
You are right that the two are not directly connected. But changes in either will change the checksum of resulting binaries.
|
Yep, that's an inconvenience. Once the I/O code has been refactored into a single file, this will change.
The Fingerprint object that is linked to all binaries is automatically generated and compiled during each build. Even with no change in the source, the build script automatically takes care of this: [ 2%] Built target COORDINATE
[ 5%] Configuring FingerPrint.cpp
[ 5%] Built target FingerPrintConfigure
[ 8%] Configuring FingerPrint.cpp
[ 10%] Building CXX object CMakeFiles/FINGERPRINT.dir/Util/FingerPrint.cpp.o
[ 10%] Built target FINGERPRINT
[ 13%] Built target GITDESCRIPTION
[ 18%] Built target IMPORT
[ 21%] Built target LOGGER
Linking CXX static library libOSRM.a
[ 45%] Built target OSRM
Linking CXX executable osrm-datastore
[ 48%] Built target osrm-datastore
Linking CXX executable osrm-extract
[ 70%] Built target osrm-extract
Linking CXX executable osrm-prepare
[ 89%] Built target osrm-prepare
Linking CXX executable osrm-routed
[100%] Built target osrm-routed As soon as the interface between files changes, i.e a file that loads or stores data from/to disk, all binaries are relinked with an updated fingerprint. |
i'm working on smarter caching of test files, so that extracting and preparing will not be run if only the osrm-routed binary changed.
but it seems that relinking will change the bins even though no source code has changed. somehow linking must involve a timestamp or random number? after running 'rake' from the root folder, all the bins have new a new sha1sum, which makes it impossible to skip the extraction/preparing step.
running osrm-extract -v shows the same version string; the git revision is unchanged, and there's no data info. so it must be something else.
The text was updated successfully, but these errors were encountered: