Skip to content

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Aug 22, 2025

This reverts commit 2f2a29b. It breaks the option hashing in our CI script.

This reverts commit 2f2a29b. It
breaks the option hashing in our CI script.
@hageboeck
Copy link
Member

hageboeck commented Aug 22, 2025

This reverts commit 2f2a29b. It breaks the option hashing in our CI script.

While I agree that having removed these options changes the hash, does it also lead to clashes? sha("<fewer defaults>" + <a few special options>) should still lead to pretty unique hashes in comparison to ("<constant set of defaults>" + "<a few special options>"), shouldn't it?

Sorry @dpiparo, but I have to say it again 😄:

Maybe we should invest a few extra minutes of compile time to build from scratch with a big ccache. This would make all previous-build-folder-related problems go away and simplify the CI scripts.

@hahnjo
Copy link
Member Author

hahnjo commented Aug 22, 2025

This reverts commit 2f2a29b. It breaks the option hashing in our CI script.

While I agree that having removed these options changes the hash, does it also lead to clashes? sha("<fewer defaults>" + <a few special options>) should still lead to pretty unique hashes in comparison to ("<constant set of defaults>" + "<a few special options>"), shouldn't it?

I'm not worried about clashes between platforms, but the problem is that changing a default in cmake/modules/RootBuildOptions.cmake will not change the hash and our CI will attempt an incremental build. That's why (as far as I understand), explicit spelling of all options was added in the first place.

@hageboeck
Copy link
Member

I'm not worried about clashes between platforms, but the problem is that changing a default in cmake/modules/RootBuildOptions.cmake will not change the hash and our CI will attempt an incremental build. That's why (as far as I understand), explicit spelling of all options was added in the first place.

This makes total sense, but you also named a possible solution: 🙂
Hash the options string + cmake/modules/RootBuildOptions.cmake

(Or we don't do incremental builds. 😉)

Copy link

Test Results

    21 files      21 suites   3d 13h 32m 57s ⏱️
 3 560 tests  3 247 ✅   0 💤 313 ❌
72 956 runs  72 332 ✅ 311 💤 313 ❌

For more details on these failures, see this check.

Results for commit 5832be3.

@guitargeek
Copy link
Contributor

guitargeek commented Aug 22, 2025

I'm not worried about clashes between platforms, but the problem is that changing a default in cmake/modules/RootBuildOptions.cmake will not change the hash and our CI will attempt an incremental build. That's why (as far as I understand), explicit spelling of all options was added in the first place.

This problem is that when someone changes RootBuildOptions.cmake, the risk that they forget to update globals.txt is high and as a result, the CI config uncontrollably diverges from the default config over time.

Stephans proposed solutions both make sense to me! The drawback of hashing RootBuildOptions.cmake is that we get false positives when someone just changes a typo in that file or something, but that's rare and most changes in that file should better result in a clean rebuild anyway.

@guitargeek
Copy link
Contributor

guitargeek commented Aug 22, 2025

Coincidentally, the failues in ubuntu2510 show us another potential problem with the incremental builds, because of a glibc update:

2025-08-22T06:22:41.0697191Z   Possible C++ standard library mismatch, compiled with __GLIBCXX__ '20250804'
2025-08-22T06:22:41.0697518Z   Extraction of runtime standard library version was: '20250812'

@ferdymercury
Copy link
Collaborator

ferdymercury commented Sep 1, 2025

Hash the options string + cmake/modules/RootBuildOptions.cmake

cmake has an option to print all current options, so no need to concatenate maybe?
https://stackoverflow.com/questions/16851084/how-to-list-all-cmake-build-options-and-their-default-values

That way it will not be sensitive to eg just adding a newline to that file

@hahnjo
Copy link
Member Author

hahnjo commented Sep 1, 2025

Hash the options string + cmake/modules/RootBuildOptions.cmake

cmake has an option to print all current options, so no need to concatenate maybe?

We need the hash before actually running cmake, to download the "right" binary artifacts in PR builds.

@ferdymercury
Copy link
Collaborator

ferdymercury commented Sep 1, 2025

We need the hash before actually running cmake, to download the "right" binary artifacts in PR builds.

Couldn't you do:

mkdir -p /tmp/aux && cd /tmp/aux
cmake /path/to/src -Dwhatever-os-options
cmake -L > /tmp/currentoptions.txt
rm -r /tmp/aux
Download hash matching currentoptions.txt

Or is that adding too much time delay due to the extra temporary configure step?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants