Depends: Fix Qt's rcc determinism #13732
Conversation
|
I assume that this can be solved by adding rcc into faketime wrapper. |
|
@DrahtBot Have some fun here! |
|
Concept ACK. Nice first time-contribution. Thanks! Determinism is our friend! |
|
adding I don't think that would be a viable alternative anyways, as we would then need to ensure that the last modified timestamp of every generated There is an alternative that is less code than this, but I'm not sure if it would be acceptable either as it could potentially interfere with non-gitian builds: --- src/Makefile.qt.include
+++ src/Makefile.qt.include
@@ -430,7 +430,7 @@
$(QT_QRC_LOCALE_CPP): $(QT_QRC_LOCALE) $(QT_QM)
@test -f $(RCC)
@cp -f $< $(@D)/temp_$(<F)
- $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(RCC) -name bitcoin_locale $(@D)/temp_$(<F) | \
+ $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(RCC) --format-version 1 -name bitcoin_locale $(@D)/temp_$(<F) | \
$(SED) -e '/^\*\*.*Created:/d' -e '/^\*\*.*by:/d' > $@
@rm $(@D)/temp_$(<F)
This is a runtime option to |
|
Indeed, looks like the results are stable now. |
Note to reviewers: This pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK |
|
@laanwj In this particular case, upstream Qt did indeed "fix" the issue, but only for versions 5.11+. Even though the QTBUG was closed and marked invalid, a proposed change addressing the issue was merged. (qt/qtbase@38271e9) I agree though, it would be great if the patch made it into Qt's 5.9 branch and a 5.9.7 version released so it wouldn't need to be explicitely applied in depends here. Until then, however, I think this is the best that can be done while still keeping Qt 5.9. |
depends/packages/qt.mk
Outdated
There was a problem hiding this comment.
We need to export the variable here as well so that non-gitian builds get stable results. See below where QT_RCC_TEST is set to 1.
There was a problem hiding this comment.
Noted, will add this soon
There was a problem hiding this comment.
swapped out the build_env variable further up in this file and didn't notice any break in gitian results determinism.
There was a problem hiding this comment.
QT_RCC_TEST was the variable for this in the past. If QT_RCC_SOURCE_DATE_OVERRIDE is the new one, let's just replace QT_RCC_TEST. I assume there's no need for both.
Same goes for the other descriptors as well.
There was a problem hiding this comment.
removed QT_RCC_TEST=1 from gitian descriptors and didn't notice any break in gitian results determinism.
|
@laanwj Yes, ideally qt's configure would have an option for disabling timestamps by default, which is quickly becoming the norm for host-side build tools. |
Fuzzbawls
force-pushed
the
btc_fix-rcc-determinism
branch
from
927bb45 to
cc4a5d1
Compare
|
Rebased and pushed to incorporate feedback from @theuni. One thing of note: While gitian builds are now deterministic, non-gitian builds that use some further tweaking to |
|
@Fuzzbawls Sorry to do this to you, but I just had a look at the qt source, and it turns out that RCC_TEST has a totally different function: it serves as the seed for a hash function that ends up dictating the order of file entries. So my request above was completely wrong, we need both :( Would you mind putting it back? utACK after that. |
Backport of https://bugreports.qt.io/browse/QTBUG-62511 to resolve locale determinism during the build process.
Fuzzbawls
force-pushed
the
btc_fix-rcc-determinism
branch
from
cc4a5d1 to
6b5506a
Compare
|
@theuni Thanks...I had a feeling it may still be needed...just hadn't had the time to do extensive build testing. I've reverted to re-add the There is still an outstanding issue of non-gitian builds (that use a depends target) producing inconsistent results with RCC...perhaps that can be it's own issue/PR? |
|
Gitian builds for commit ef4fac0 (master):
Gitian builds for commit c2fbdcd7d926841e34d5d5b24be3da781fbc48fc (master and this pull):
|
|
utACK 6b5506a |
|
Gitian builds for commit ef4fac0 (master):
Gitian builds for commit c2fbdcd7d926841e34d5d5b24be3da781fbc48fc (master and this pull):
|
6b5506a Fix Qt's rcc determinism for depends/gitian (Fuzzbawls) Pull request description: With the update to Qt 5.9 having been merged, Qt's `rcc` tool now embeds a file's last modified time in it's output. Since the build system generates temporary files for all locale translations (`*.qm` files) at build time, the resulting `qrc_bitcoin_locale.cpp` file was always being generated in a non-deterministic way. This is a backport of https://bugreports.qt.io/browse/QTBUG-62511, which is included in Qt versions 5.11+, that allows for an environment variable (`QT_RCC_SOURCE_DATE_OVERRIDE`) to override the behavior described above. This environment variable is in turn set in the gitian descriptors, as that is where determinism is vital for release purposes. Prior to this, the `qt_libbitcoinqt_a-qrc_bitcoin_locale.o` object file (included into `libbitcoinqt.a`) was returning a different `sha256sum` for each and every build, regardless of file contents change, thus breaking determinism in the resulting binaries. This should fix #13731 Tree-SHA512: 174017e41f9afc3950ef54a9419de81577ec900db9aec3c78ccd3d879c6aecaaeb944fde0615b933f43e6ca9d7898a27ec071cdd0b91cb772755a3012de96725
Backport of https://bugreports.qt.io/browse/QTBUG-62511 to resolve locale determinism during the build process.
a58868d build: Makes rcc output always deterministic (Hennadii Stepanov) Pull request description: The Qt Resource Compiler ([rcc](https://doc.qt.io/qt-5/rcc.html)) has a command-line option `--format-version` which has the [default value](https://code.qt.io/cgit/qt/qtbase.git/tree/src/tools/rcc/main.cpp?h=5.12.10#n172) 2. The only difference from `--format-version 1` is adding a [last modified timestamp](https://code.qt.io/cgit/qt/qtbase.git/tree/src/tools/rcc/rcc.cpp?h=5.12.10#n207) to the output file ([credits](#21654 (comment)) to **fanquake**). That, in turn, forces us to use `QT_RCC_SOURCE_DATE_OVERRIDE=1` to get deterministic builds (#13732). This change makes rcc output always deterministic by using `--format-version 1` option that makes usage of the `QT_RCC_SOURCE_DATE_OVERRIDE` needless. --- Also it improves interaction with ccache: On master (f6c44e9): ``` $ make && make clean && ccache --zero-stats && make && ccache --show-stats ... cache directory /home/hebasto/.ccache primary config /home/hebasto/.ccache/ccache.conf secondary config (readonly) /etc/ccache.conf stats updated Sun Apr 11 15:45:43 2021 stats zeroed Sun Apr 11 15:45:05 2021 cache hit (direct) 638 cache hit (preprocessed) 0 cache miss 1 cache hit rate 99.84 % called for link 10 cleanups performed 0 files in cache 20023 cache size 13.2 GB max cache size 15.0 GB ``` The missed file is always `qt/libbitcoinqt_a-qrc_bitcoin_locale.o`. With this PR: ``` $ make && make clean && ccache --zero-stats && make && ccache --show-stats ... cache directory /home/hebasto/.ccache primary config /home/hebasto/.ccache/ccache.conf secondary config (readonly) /etc/ccache.conf stats updated Sun Apr 11 15:28:46 2021 stats zeroed Sun Apr 11 15:28:21 2021 cache hit (direct) 639 cache hit (preprocessed) 0 cache miss 0 cache hit rate 100.00 % called for link 10 cleanups performed 0 files in cache 20012 cache size 13.2 GB max cache size 15.0 GB ``` ACKs for top commit: fanquake: ACK a58868d Tree-SHA512: 52f4a3267f41883d13025c0de79b6da22e92d60c729e01b986935c6812bbfe7fadc40b742bd715bfdf09df94af6838d4fbbe8208c6123f366108e38c8e1121c5
Backport of https://bugreports.qt.io/browse/QTBUG-62511 to resolve locale determinism during the build process.
9 participants
With the update to Qt 5.9 having been merged, Qt's
rcctool now embeds a file's last modified time in it's output. Since the build system generates temporary files for all locale translations (*.qmfiles) at build time, the resultingqrc_bitcoin_locale.cppfile was always being generated in a non-deterministic way.This is a backport of https://bugreports.qt.io/browse/QTBUG-62511, which is included in Qt versions 5.11+, that allows for an environment variable (
QT_RCC_SOURCE_DATE_OVERRIDE) to override the behavior described above. This environment variable is in turn set in the gitian descriptors, as that is where determinism is vital for release purposes.Prior to this, the
qt_libbitcoinqt_a-qrc_bitcoin_locale.oobject file (included intolibbitcoinqt.a) was returning a differentsha256sumfor each and every build, regardless of file contents change, thus breaking determinism in the resulting binaries.This should fix #13731