depends: Export variables from make to environment explicitly #19882

This PR makes possible to integrate other Qt modules (like QtQml or QtSvg) into our static builds, and fixes bug:

Thanks for that stab. Having an error when cross-compiling it for macOS:

$ make -C depends HOST=x86_64-apple-darwin16
...
make[3]: x86_64-apple-darwin16-ar: Command not found
make[3]: *** [Makefile:936: ../../lib/libQt5Qml.a] Error 127

@hebasto I hit the same error. I'll take a look.

The current gitian binaries remains the same, e.g.:

$ diffoscope x86_64-linux-gnu/a/bin/bitcoind x86_64-linux-gnu/b/bin/bitcoind
--- x86_64-linux-gnu/a/bin/bitcoind
+++ x86_64-linux-gnu/b/bin/bitcoind
├── readelf --wide --notes {}
│ @@ -1,15 +1,15 @@
│  
│  Displaying notes found in: .note.ABI-tag
│    Owner                Data size 	Description
│    GNU                  0x00000010	NT_GNU_ABI_TAG (ABI version tag)	    OS: Linux, ABI: 3.2.0
│  
│  Displaying notes found in: .note.gnu.build-id
│    Owner                Data size 	Description
│ -  GNU                  0x00000014	NT_GNU_BUILD_ID (unique build ID bitstring)	    Build ID: c0a64b5bca8f64dd545a358f340453c12d22b08e
│ +  GNU                  0x00000014	NT_GNU_BUILD_ID (unique build ID bitstring)	    Build ID: 358725c838441cb162c57fb89dbbd655b6c68c19
│  
│  Displaying notes found in: .note.stapsdt
│    Owner                Data size 	Description
│    stapsdt              0x00000036	NT_STAPSDT (SystemTap probe descriptors)	    Provider: libstdcxx
│      Name: throw
│      Location: 0x00000000008e469d, Base: 0x0000000000aea358, Semaphore: 0x0000000000000000
│      Arguments: 8@%rdi 8@%rsi
├── readelf --wide --decompress --hex-dump=.rodata {}
│ @@ -38747,16 +38747,16 @@
│    0x00a97580 466f726d 61744172 672a2c20 696e7426 FormatArg*, int&
│    0x00a97590 2c20696e 74290000 00000000 00000000 , int)..........
│    0x00a975a0 43726561 74654261 73654368 61696e50 CreateBaseChainP
│    0x00a975b0 6172616d 73000000 00000000 00000000 arams...........
│    0x00a975c0 636f6e73 74204342 61736543 6861696e const CBaseChain
│    0x00a975d0 50617261 6d732620 42617365 50617261 Params& BasePara
│    0x00a975e0 6d732829 00536174 6f736869 0076302e ms().Satoshi.v0.
│ -  0x00a975f0 32302e39 392e302d 32336433 61653761 20.99.0-23d3ae7a
│ -  0x00a97600 63000000 00000000 00000000 00000000 c...............
│ +  0x00a975f0 32302e39 392e302d 30333032 34303030 20.99.0-03024000
│ +  0x00a97600 36000000 00000000 00000000 00000000 6...............
│    0x00a97610 00000000 00000000 00000000 00000000 ................
│    0x00a97620 766f6964 2074696e 79666f72 6d61743a void tinyformat:
│    0x00a97630 3a646574 61696c3a 3a466f72 6d617441 :detail::FormatA
│    0x00a97640 72673a3a 666f726d 61742873 74643a3a rg::format(std::
│    0x00a97650 6f737472 65616d26 2c20636f 6e737420 ostream&, const 
│    0x00a97660 63686172 2a2c2063 6f6e7374 20636861 char*, const cha
│    0x00a97670 722a2c20 696e7429 20636f6e 73740000 r*, int) const..
├── readelf --wide --decompress --hex-dump=.gnu_debuglink {}
│ @@ -1,5 +1,5 @@
│  
│  Hex dump of section '.gnu_debuglink':
│    0x00000000 62697463 6f696e64 2e646267 00000000 bitcoind.dbg....
│ -  0x00000010 7c5d7cfa                            |]|.
│ +  0x00000010 2e8c009

$ diffoscope win64/a/bin/bitcoind.exe win64/b/bin/bitcoind.exe 
--- win64/a/bin/bitcoind.exe
+++ win64/b/bin/bitcoind.exe
@@ -7,15 +7,15 @@
 00000060: 7420 6265 2072 756e 2069 6e20 444f 5320  t be run in DOS 
 00000070: 6d6f 6465 2e0d 0d0a 2400 0000 0000 0000  mode....$.......
 00000080: 5045 0000 6486 0d00 0000 0000 0016 9700  PE..d...........
 00000090: 0000 0000 f000 2e00 0b02 021e 0056 7d00  .............V}.
 000000a0: 0010 9700 00ca 0000 0015 0000 0010 0000  ................
 000000b0: 0000 4000 0000 0000 0010 0000 0002 0000  ..@.............
 000000c0: 0400 0000 0000 0000 0600 0100 0000 0000  ................
-000000d0: 0060 9800 0004 0000 3d2e 9700 0300 6001  .`......=.....`.
+000000d0: 0060 9800 0004 0000 4cb1 9700 0300 6001  .`......L.....`.
 000000e0: 0000 2000 0000 0000 0010 0000 0000 0000  .. .............
 000000f0: 0000 1000 0000 0000 0010 0000 0000 0000  ................
 00000100: 0000 0000 1000 0000 0070 9700 3f07 0000  .........p..?...
 00000110: 0080 9700 8033 0000 00e0 9700 a804 0000  .....3..........
 00000120: 00a0 8a00 0483 0300 0000 0000 0000 0000  ................
 00000130: 00f0 9700 845e 0000 0000 0000 0000 0000  .....^..........
 00000140: 0000 0000 0000 0000 0000 0000 0000 0000  ................
@@ -539026,16 +539026,16 @@
 00839910: 6965 7273 2069 6e20 666f 726d 6174 2073  iers in format s
 00839920: 7472 696e 6700 0000 7469 6e79 666f 726d  tring...tinyform
 00839930: 6174 3a20 546f 6f20 6d61 6e79 2063 6f6e  at: Too many con
 00839940: 7665 7273 696f 6e20 7370 6563 6966 6965  version specifie
 00839950: 7273 2069 6e20 666f 726d 6174 2073 7472  rs in format str
 00839960: 696e 6700 6d5f 666f 726d 6174 496d 706c  ing.m_formatImpl
 00839970: 0028 003b 2000 2900 5361 746f 7368 6900  .(.; .).Satoshi.
-00839980: 7630 2e32 302e 3939 2e30 2d32 3364 3361  v0.20.99.0-23d3a
-00839990: 6537 6163 0000 0000 0000 0000 0000 0000  e7ac............
+00839980: 7630 2e32 302e 3939 2e30 2d30 3330 3234  v0.20.99.0-03024
+00839990: 3030 3036 0000 0000 0000 0000 0000 0000  0006............
 008399a0: 6261 7369 635f 7374 7269 6e67 3a3a 6174  basic_string::at
 008399b0: 3a20 5f5f 6e20 2877 6869 6368 2069 7320  : __n (which is 
 008399c0: 257a 7529 203e 3d20 7468 6973 2d3e 7369  %zu) >= this->si
 008399d0: 7a65 2829 2028 7768 6963 6820 6973 2025  ze() (which is %
 008399e0: 7a75 2900 0000 0000 0000 0000 0000 0000  zu).............
 008399f0: 6765 6e65 7269 6300 7379 7374 656d 004d  generic.system.M
 00839a00: 6573 7361 6765 2074 6578 7420 756e 6176  essage text unav
@@ -616154,15 +616154,15 @@
 00966d90: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00966da0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00966db0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00966dc0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00966dd0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00966de0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00966df0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
-00966e00: 0000 0000 9725 525f 0000 0000 cc71 9700  .....%R_.....q..
+00966e00: 0000 0000 0d40 535f 0000 0000 cc71 9700  .....@S_.....q..
 00966e10: 0100 0000 2a00 0000 2a00 0000 2870 9700  ....*...*...(p..
 00966e20: d070 9700 7871 9700 1073 4100 2072 4100  .p..xq...sA. rA.
 00966e30: 6074 4100 60b2 7d00 a072 4100 a06a 4100  `tA.`.}..rA..jA.
 00966e40: c06a 4100 6073 4100 606a 4100 f090 4100  .jA.`sA.`jA...A.
 00966e50: f074 4100 9074 4100 d087 4100 108a 4100  .tA..tA...A...A.
 00966e60: 108e 4100 1091 4100 f085 4100 e087 4100  ..A...A...A...A.
 00966e70: 0076 4100 3079 4100 208a 4100 208e 4100  .vA.0yA. .A. .A.

tACK https://github.com/bitcoin/bitcoin/pull/19882/commits/6315002a359f9f6457926dd039e968a89418ad33

I can confirm this fixes OSX cross-compilation issues for https://github.com/icota/bitcoin/commit/683710e072eb90296ae5a57f53bce7cac799fc41

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22283 (build: Replace $(AT) with .SILENT by dgoncharov)
• #22126 (build: Disable make builtin rules. by dgoncharov)
• #19952 (build, ci: Add file-based logging for individual packages by hebasto)

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.

From #bitcoin-builds IRC meeting:

<cfields> hebasto: hmm, that looks reasonable. Will review and ACK/NACK. <dongcarl> I'm confused as to why this changes anything <cfields> I tried to avoid exporting as much as possible to avoid affecting sub-shells, but I think doing PATH like that should be fine. <dongcarl> Oh I see. <hebasto> dongcarl: PATH within make, and PATH in environment are distinct <cfields> dongcarl: vars don't get re-exported to subshells when set like "FOO=1 ./bar" @theuni @dongcarl Mind leaving a comment here?

I'm wondering if we can make this more generic, by keeping the $(1)_{config,build,stage}_env variables, and doing export $$($(1)_config_env) in $($(1)_configured) and friends. What do you think?

@dongcarl

I'm wondering if we can make this more generic, by keeping the $(1)_{config,build,stage}_env variables, and doing export $$($(1)_config_env) in $($(1)_configured) and friends. What do you think?

I suppose that only $PATH variable is required to be exported from within make to the shell environment. https://www.gnu.org/software/make/manual/make.html#Variables_002fRecursion

@hebasto I'm not quite sure what you mean, we would want things like PKG_CONFIG_LIBDIR, PKG_CONFIG_PATH, and CMAKE_MODULE_PATH to also be set for all commands specified in $(1)_config_cmds no?

@dongcarl

@hebasto I'm not quite sure what you mean, we would want things like PKG_CONFIG_LIBDIR, PKG_CONFIG_PATH, and CMAKE_MODULE_PATH to also be set for all commands specified in $(1)_config_cmds no?

Correct. $(1)_config_cmds does not use MAKE variable, therefore all required variables should be exported explicitly. Do you want me update this pull in a more generic way?

@hebasto Yup! Let's do it in a more generic way. We can also get rid of the $($(1)_build_env) prefix in all those recipe calls :-)

Updated 6315002a359f9f6457926dd039e968a89418ad33 -> 1194739baef62e15c1e600fa8839e31fa98e324c (pr19882.01 -> pr19882.02, diff):

• rebased

• addressed @dongcarl's comment:

I'm wondering if we can make this more generic, by keeping the $(1)_{config,build,stage}_env variables, and doing export $$($(1)_config_env) in $($(1)_configured) and friends.

<cfields> dongcarl: vars don't get re-exported to subshells when set like "FOO=1 ./bar"

But... they're supposed to...?

This does not seem to work for QtDeclarative(QtQml & QtQuick) and QtQuickControls2. Has anyone successfully added and tested the complete dependency chain to enable #16883 static dependency builds?

@BlockMechanic

This does not seem to work for QtDeclarative(QtQml & QtQuick) and QtQuickControls2.

What issues or errors have you encountered with this PR?

Has anyone successfully added and tested the complete dependency chain to enable #16883 static dependency builds?

I think @icota has (https://github.com/bitcoin/bitcoin/pull/19882#issuecomment-687609534). And, of course, more testing from reviewers are required yet. Maybe @promag could test it?

FWIW, I've rebased this pull on top of master (86f20071931b803b5f26ed8f685d98d4919fb7a7) + 695da7ea9d3faf14a0f83933a88c095fa23dfd93 for testing.

@BlockMechanic

This does not seem to work for QtDeclarative(QtQml & QtQuick) and QtQuickControls2.

What issues or errors have you encountered with this PR?

Has anyone successfully added and tested the complete dependency chain to enable #16883 static dependency builds?

I think @icota has (#19882 (comment)). And, of course, more testing from reviewers are required yet. Maybe @promag could test it?

FWIW, I've rebased this pull on top of master (86f2007) + 695da7e for testing.

Hi

Yeah ,i tested it myself, it does not work for ALL Qt modules, only some. It definitely does not work for QtQuickControls2

@BlockMechanic

Yeah ,i tested it myself, it does not work for ALL Qt modules, only some. It definitely does not work for QtQuickControls2

What issues or errors have you encountered? Maybe an excerpt from the build log?

@fanquake if this commit could be useful for #20504, feel free to cherry-pick it.

Closing, as the initial task of this pull remains unsolved (thanks to @BlockMechanic for pointing it out).

@BlockMechanic

This does not seem to work for QtDeclarative(QtQml & QtQuick) and QtQuickControls2. Has anyone successfully added and tested the complete dependency chain to enable #16883 static dependency builds?

Yeah ,i tested it myself, it does not work for ALL Qt modules, only some. It definitely does not work for QtQuickControls2

It works for QtDeclarative(QtQml & QtQuick) and QtQuickControls2 in the hebasto:201213-top-DEMO demo branch which is built on top of the #20641.

CI task: https://cirrus-ci.com/task/5614122638245888

@icota @dongcarl @BlockMechanic @fanquake

Reopened as it is still required (see #20641), and rebased 1194739baef62e15c1e600fa8839e31fa98e324c -> 9a3194cd666b390bc4c520f8291ab2b673061cae (pr19882.02 -> pr19882.03).

We talked about this offline a bit, @theuni here's a demo of how it's broken right now: https://github.com/dongcarl/bitcoin/commit/e233c6d1e96ed35489c6be37f07bd01546083b9e

After today's discussion with @theuni and @dongcarl the less invasive alternative solution, which was suggested by @theuni, actually fixes the initial bug described in the PR description:

--- a/depends/packages/qt.mk
+++ b/depends/packages/qt.mk
@@ -258,6 +258,7 @@ define $(package)_build_cmds
 endef
 
 define $(package)_stage_cmds
+  export PATH && \
   $(MAKE) -C qtbase/src INSTALL_ROOT=$($(package)_staging_dir) $(addsuffix -install_subtargets,$(addprefix sub-,$($(package)_qt_libs))) && \
   $(MAKE) -C qttools/src/linguist INSTALL_ROOT=$($(package)_staging_dir) $(addsuffix -install_subtargets,$(addprefix sub-,$($(package)_linguist_tools))) && \
   $(MAKE) -C qttranslations INSTALL_ROOT=$($(package)_staging_dir) install_subtargets

I've used this new approach in https://github.com/bitcoin-core/gui-qml/pull/19.

@dongcarl Maybe put your nice bug demo into a new issue, and close this one?

Opened #22719, you can close this one if you think it appropriate!