depends: Export variables from make to environment explicitly

hebasto commented at 12:36 pm on September 5, 2020: member

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:
0$ make -C depends HOST=x86_64-apple-darwin16
1...
2make[3]: x86_64-apple-darwin16-ar: Command not found
3make[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.:

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

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

fanquake added the label Build system on Sep 5, 2020

icota commented at 1:12 pm on September 5, 2020: contributor

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

hebasto closed this on Sep 6, 2020

hebasto reopened this on Sep 6, 2020

DrahtBot commented at 1:54 pm on September 19, 2020: member

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

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.

hebasto commented at 8:12 am on September 21, 2020: member

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?

dongcarl commented at 3:28 pm on September 22, 2020: member

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?

hebasto commented at 3:32 pm on September 22, 2020: member

@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

dongcarl commented at 3:36 pm on September 22, 2020: member

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

hebasto commented at 3:53 pm on September 22, 2020: member

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

dongcarl commented at 3:55 pm on September 22, 2020: member

@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 :-)

hebasto force-pushed on Sep 24, 2020

hebasto commented at 6:25 am on September 24, 2020: member

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.

hebasto renamed this:
~~depends: Export PATH variable rather passing it to the `call` function~~
depends: Export variables from make to environment explicitly
on Sep 24, 2020

luke-jr commented at 2:52 pm on October 24, 2020: member

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

But… they’re supposed to…?

BlockMechanic referenced this in commit 7eb9c96fc9 on Dec 8, 2020

BlockMechanic commented at 7:43 am on December 9, 2020: contributor

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?

hebasto referenced this in commit 683710e072 on Dec 10, 2020

hebasto commented at 1:09 pm on December 10, 2020: member

@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 commented at 1:23 pm on December 10, 2020: contributor

@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

hebasto commented at 1:37 pm on December 10, 2020: member

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

hebasto commented at 6:21 pm on December 10, 2020: member

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

hebasto closed this on Dec 10, 2020

hebasto referenced this in commit abb0f07fc9 on Jun 16, 2021

hebasto reopened this on Jun 16, 2021

hebasto commented at 7:17 pm on June 16, 2021: member

@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

hebasto force-pushed on Jun 16, 2021

depends: Export variables from make to environment explicitly 9a3194cd66

hebasto force-pushed on Jun 16, 2021

hebasto commented at 7:26 pm on June 16, 2021: member

@icota @dongcarl @BlockMechanic @fanquake

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

dongcarl commented at 5:25 pm on August 12, 2021: member

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

hebasto commented at 7:36 pm on August 12, 2021: member

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:

0--- a/depends/packages/qt.mk
1+++ b/depends/packages/qt.mk
2@@ -258,6 +258,7 @@ define $(package)_build_cmds
3 endef
4 
5 define $(package)_stage_cmds
6+  export PATH && \
7   $(MAKE) -C qtbase/src INSTALL_ROOT=$($(package)_staging_dir) $(addsuffix -install_subtargets,$(addprefix sub-,$($(package)_qt_libs))) && \
8   $(MAKE) -C qttools/src/linguist INSTALL_ROOT=$($(package)_staging_dir) $(addsuffix -install_subtargets,$(addprefix sub-,$($(package)_linguist_tools))) && \
9   $(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?

dongcarl commented at 4:54 pm on August 16, 2021: member

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

hebasto closed this on Aug 16, 2021

hebasto added the label Up for grabs on Aug 19, 2021

hebasto removed the label Up for grabs on Apr 22, 2022

fanquake referenced this in commit 3eaf7be6ad on Dec 8, 2022

sidhujag referenced this in commit 5dde8cb2a6 on Dec 8, 2022

DrahtBot locked this on Apr 22, 2023

depends: Export variables from make to environment explicitly #19882

Conflicts