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

Depends: Fix Qt's rcc determinism #13732

Merged
merged 1 commit into from Jul 29, 2018

Conversation

Projects
None yet
9 participants
@Fuzzbawls
Contributor

Fuzzbawls commented Jul 21, 2018

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

@fanquake fanquake requested a review from theuni Jul 21, 2018

@fanquake fanquake added this to the 0.17.0 milestone Jul 21, 2018

@ken2812221

This comment has been minimized.

Show comment
Hide comment
@ken2812221

ken2812221 Jul 21, 2018

Member

I assume that this can be solved by adding rcc into faketime wrapper.

Member

ken2812221 commented Jul 21, 2018

I assume that this can be solved by adding rcc into faketime wrapper.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jul 21, 2018

Member

@DrahtBot Have some fun here!

Member

MarcoFalke commented Jul 21, 2018

@DrahtBot Have some fun here!

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Jul 21, 2018

Member

Concept ACK. Nice first time-contribution. Thanks!

Determinism is our friend!

Member

practicalswift commented Jul 21, 2018

Concept ACK. Nice first time-contribution. Thanks!

Determinism is our friend!

@Fuzzbawls

This comment has been minimized.

Show comment
Hide comment
@Fuzzbawls

Fuzzbawls Jul 21, 2018

Contributor

adding rcc to the list of faketime wrapped binaries is problematic in that the rcc binary location is also defined in the share/config.site file that gets used for gitian builds, which does not use the $PATH environment variable, and the build system isn't really set up to allow for separate paths for each individual Qt tool.

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 *.qm file is static (faketime again?).

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 rcc that forces the older format version to be used, which specifically does not encode file timestamps into the result. There was some concern about using such an override however in the Qt bug report's discussion thread.

Contributor

Fuzzbawls commented Jul 21, 2018

adding rcc to the list of faketime wrapped binaries is problematic in that the rcc binary location is also defined in the share/config.site file that gets used for gitian builds, which does not use the $PATH environment variable, and the build system isn't really set up to allow for separate paths for each individual Qt tool.

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 *.qm file is static (faketime again?).

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 rcc that forces the older format version to be used, which specifically does not encode file timestamps into the result. There was some concern about using such an override however in the Qt bug report's discussion thread.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jul 22, 2018

Member

Indeed, looks like the results are stable now.

Member

MarcoFalke commented Jul 22, 2018

Indeed, looks like the results are stable now.

@DrahtBot

This comment has been minimized.

Show comment
Hide comment
@DrahtBot

DrahtBot Jul 22, 2018

Contributor
Note to reviewers: This pull request conflicts with the following ones:
  • #13710 ([depends] Add riscv qt depends support for cross compiling bitcoin-qt by ken2812221)
  • #13696 (Add aarch64 qt depends support for cross compiling bitcoin-qt by TheCharlatan)
  • #13665 ([WIP][build] Add risc-v support to gitian by ken2812221)

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.

Contributor

DrahtBot commented Jul 22, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #13710 ([depends] Add riscv qt depends support for cross compiling bitcoin-qt by ken2812221)
  • #13696 (Add aarch64 qt depends support for cross compiling bitcoin-qt by TheCharlatan)
  • #13665 ([WIP][build] Add risc-v support to gitian by ken2812221)

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.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 22, 2018

Member

Concept ACK
Thanks a lot for figuring this out.!
In the longer run, it would be a good idea to get deterministic-build support pushed upstream (@theuni has some experience doing this for binutils).

Member

laanwj commented Jul 22, 2018

Concept ACK
Thanks a lot for figuring this out.!
In the longer run, it would be a good idea to get deterministic-build support pushed upstream (@theuni has some experience doing this for binutils).

@Fuzzbawls

This comment has been minimized.

Show comment
Hide comment
@Fuzzbawls

Fuzzbawls Jul 22, 2018

Contributor

@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.

Contributor

Fuzzbawls commented Jul 22, 2018

@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.

@@ -133,6 +133,7 @@ define $(package)_preprocess_cmds
patch -p1 -i $($(package)_patch_dir)/fix_qt_pkgconfig.patch &&\
patch -p1 -i $($(package)_patch_dir)/fix_configure_mac.patch &&\
patch -p1 -i $($(package)_patch_dir)/fix_no_printer.patch &&\
patch -p1 -i $($(package)_patch_dir)/fix_rcc_determinism.patch &&\

This comment has been minimized.

@theuni

theuni Jul 22, 2018

Member

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.

@theuni

theuni Jul 22, 2018

Member

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.

This comment has been minimized.

@Fuzzbawls

Fuzzbawls Jul 23, 2018

Contributor

Noted, will add this soon

@Fuzzbawls

Fuzzbawls Jul 23, 2018

Contributor

Noted, will add this soon

This comment has been minimized.

@Fuzzbawls

Fuzzbawls Jul 24, 2018

Contributor

swapped out the build_env variable further up in this file and didn't notice any break in gitian results determinism.

@Fuzzbawls

Fuzzbawls Jul 24, 2018

Contributor

swapped out the build_env variable further up in this file and didn't notice any break in gitian results determinism.

@@ -43,6 +43,7 @@ script: |
HOST_LDFLAGS=-static-libstdc++
export QT_RCC_TEST=1
export QT_RCC_SOURCE_DATE_OVERRIDE=1

This comment has been minimized.

@theuni

theuni Jul 22, 2018

Member

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.

@theuni

theuni Jul 22, 2018

Member

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.

This comment has been minimized.

@Fuzzbawls

Fuzzbawls Jul 24, 2018

Contributor

removed QT_RCC_TEST=1 from gitian descriptors and didn't notice any break in gitian results determinism.

@Fuzzbawls

Fuzzbawls Jul 24, 2018

Contributor

removed QT_RCC_TEST=1 from gitian descriptors and didn't notice any break in gitian results determinism.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jul 22, 2018

Member

@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.

Member

theuni commented Jul 22, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@Fuzzbawls

Fuzzbawls Jul 24, 2018

Contributor

Rebased and pushed to incorporate feedback from @theuni.

One thing of note: While gitian builds are now deterministic, non-gitian builds that use --prefix= or CONFIG_SITE= pointing at a depends path are only deterministic if they also set QT_RCC_SOURCE_DATE_OVERRIDE=1 at compile time

CONFIG_SITE=/home/fuzzbawls/bitcoin/depends/x86_64-unknown-linux/share/config.site ./configure
QT_RCC_SOURCE_DATE_OVERRIDE=1 make

some further tweaking to depends/config.site.in and/or depends/funcs.mk...or configure.ac may be in order to automatically export that environment variable for non-gitian builds, but that may be broadening the scope just a bit too much here given the upcoming release cycle.

Contributor

Fuzzbawls commented Jul 24, 2018

Rebased and pushed to incorporate feedback from @theuni.

One thing of note: While gitian builds are now deterministic, non-gitian builds that use --prefix= or CONFIG_SITE= pointing at a depends path are only deterministic if they also set QT_RCC_SOURCE_DATE_OVERRIDE=1 at compile time

CONFIG_SITE=/home/fuzzbawls/bitcoin/depends/x86_64-unknown-linux/share/config.site ./configure
QT_RCC_SOURCE_DATE_OVERRIDE=1 make

some further tweaking to depends/config.site.in and/or depends/funcs.mk...or configure.ac may be in order to automatically export that environment variable for non-gitian builds, but that may be broadening the scope just a bit too much here given the upcoming release cycle.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jul 24, 2018

Member

@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.

Member

theuni commented Jul 24, 2018

@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.

Fix Qt's rcc determinism for depends/gitian
Backport of https://bugreports.qt.io/browse/QTBUG-62511 to resolve
locale determinism during the build process.
@Fuzzbawls

This comment has been minimized.

Show comment
Hide comment
@Fuzzbawls

Fuzzbawls Jul 26, 2018

Contributor

@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 QT_RCC_TEST=1 env vars where appropriate and so far gitian builds are looking nice and consistent.

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?

Contributor

Fuzzbawls commented Jul 26, 2018

@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 QT_RCC_TEST=1 env vars where appropriate and so far gitian builds are looking nice and consistent.

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?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli
Member

jonasschnelli commented Jul 27, 2018

Looks good.
Gitian Build:
https://bitcoin.jonasschnelli.ch/build/719

@bitcoin bitcoin deleted a comment from DrahtBot Jul 28, 2018

@bitcoin bitcoin deleted a comment from DrahtBot Jul 28, 2018

@DrahtBot

This comment has been minimized.

Show comment
Hide comment
@DrahtBot

DrahtBot Jul 29, 2018

Contributor

Gitian builds for commit ef4fac0 (master):

Gitian builds for commit c2fbdcd (master and this pull):

Contributor

DrahtBot commented Jul 29, 2018

Gitian builds for commit ef4fac0 (master):

Gitian builds for commit c2fbdcd (master and this pull):

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke
Member

MarcoFalke commented Jul 29, 2018

utACK 6b5506a

@DrahtBot

This comment has been minimized.

Show comment
Hide comment
@DrahtBot

DrahtBot Jul 29, 2018

Contributor

Gitian builds for commit ef4fac0 (master):

Gitian builds for commit c2fbdcd (master and this pull):

Contributor

DrahtBot commented Jul 29, 2018

Gitian builds for commit ef4fac0 (master):

Gitian builds for commit c2fbdcd (master and this pull):

@MarcoFalke MarcoFalke merged commit 6b5506a into bitcoin:master Jul 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Jul 29, 2018

Merge #13732: Depends: Fix Qt's rcc determinism
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment