depends: Export variables from make to environment explicitly #19882
Conversation
|
tACK 6315002 I can confirm this fixes OSX cross-compilation issues for icota@683710e |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
|
From #bitcoin-builds IRC meeting:
|
|
I'm wondering if we can make this more generic, by keeping the |
I suppose that only $PATH variable is required to be exported from within make to the shell environment. |
|
@hebasto I'm not quite sure what you mean, we would want things like |
Correct. |
|
@hebasto Yup! Let's do it in a more generic way. We can also get rid of the |
hebasto
force-pushed
the
200905-build
branch
from
6315002 to
1194739
Compare
|
Updated 6315002 -> 1194739 (pr19882.01 -> pr19882.02, diff):
|
hebasto
changed the title
call function
But... they're supposed to...? |
Temp revert, bitcoin#19882
|
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? |
What issues or errors have you encountered with this PR?
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 |
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). |
It works for QtDeclarative(QtQml & QtQuick) and QtQuickControls2 in the hebasto:201213-top-DEMO demo branch which is built on top of the #20641. |
|
@icota @dongcarl @BlockMechanic @fanquake Reopened as it is still required (see #20641), and rebased 1194739 -> 9a3194c (pr19882.02 -> pr19882.03). |
|
We talked about this offline a bit, @theuni here's a demo of how it's broken right now: dongcarl@e233c6d |
|
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_subtargetsI've used this new approach in bitcoin-core/gui-qml#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! |
… to all `$(package)_*_cmds` affbf58 build: Move environment variables into `$(package)_config_env` (Hennadii Stepanov) d44fcd3 build: Make $(package)_*_env available to all $(package)_*_cmds (Hennadii Stepanov) Pull request description: On master (1e7564e) the depends build system, which is based on pure GNU Make, works, but it lacks robustness, and in some corner cases it fails. For example, see bitcoin/bitcoin#22552. Another [bug](bitcoin/bitcoin#22719) in the depends build system has already become a problem at least two times in the past (bitcoin/bitcoin#16883 (comment) and bitcoin/bitcoin#24134). Each time the problem was solved with other means. The initial [solution](bitcoin/bitcoin#19882) had some discussion. Also it was discussed on the IRC meeting in #bitcoin-core-builds channel. This PR, actually, is a resurrection of it, as the bug silently struck pretty [recently](bitcoin/bitcoin#24134). The bug is well described in bitcoin/bitcoin#22719. Here is another, a bit simpler description, which requires only basic shell (bash, dash etc) experience. After creating targets by this code:https://github.com/bitcoin/bitcoin/blob/1e7564eca8a688f39c75540877ec3bdfdde766b1/depends/funcs.mk#L280 a "draft" line of recipe like `$($(1)_config_env) $(call $(1)_config_cmds, $(1))` becomes a shell command sequence `VAR1=foo VAR2=bar command1 && command2` which is supposed to be executed in a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution). Please note that `VAR1=foo VAR2=bar` part is visible for the first `command1` only (for details see shell docs). Example: ```sh $ VAR1="foo" VAR2="bar" echo "begin" && printenv VAR1 && printenv VAR2 && echo "end" begin $ echo $? 1 ``` Using the `export` command is a trivial solution: ```sh $ export VAR1="foo" VAR2="bar"; echo "begin" && printenv VAR1 && printenv VAR2 && echo "end" begin foo bar end $ echo $? 0 ``` As a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution) is invoked for each line of the recipe, there are no side effects of using `export`. It means this solution should not be considered invasive. Fixes bitcoin/bitcoin#22719. --- Also this PR removes no longer needed crutch from `qt.mk`. ACKs for top commit: fanquake: ACK affbf58 Tree-SHA512: 0ce2cf82870a7774bdf1592fac50857126ae47da902e349f1092d50109223be9d6a8efd5e92ec08c2ca775b17516482aabaf232378950ade36484a883acc177b
…`$(package)_*_cmds` affbf58 build: Move environment variables into `$(package)_config_env` (Hennadii Stepanov) d44fcd3 build: Make $(package)_*_env available to all $(package)_*_cmds (Hennadii Stepanov) Pull request description: On master (1e7564e) the depends build system, which is based on pure GNU Make, works, but it lacks robustness, and in some corner cases it fails. For example, see bitcoin#22552. Another [bug](bitcoin#22719) in the depends build system has already become a problem at least two times in the past (bitcoin#16883 (comment) and bitcoin#24134). Each time the problem was solved with other means. The initial [solution](bitcoin#19882) had some discussion. Also it was discussed on the IRC meeting in #bitcoin-core-builds channel. This PR, actually, is a resurrection of it, as the bug silently struck pretty [recently](bitcoin#24134). The bug is well described in bitcoin#22719. Here is another, a bit simpler description, which requires only basic shell (bash, dash etc) experience. After creating targets by this code:https://github.com/bitcoin/bitcoin/blob/1e7564eca8a688f39c75540877ec3bdfdde766b1/depends/funcs.mk#L280 a "draft" line of recipe like `$($(1)_config_env) $(call $(1)_config_cmds, $(1))` becomes a shell command sequence `VAR1=foo VAR2=bar command1 && command2` which is supposed to be executed in a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution). Please note that `VAR1=foo VAR2=bar` part is visible for the first `command1` only (for details see shell docs). Example: ```sh $ VAR1="foo" VAR2="bar" echo "begin" && printenv VAR1 && printenv VAR2 && echo "end" begin $ echo $? 1 ``` Using the `export` command is a trivial solution: ```sh $ export VAR1="foo" VAR2="bar"; echo "begin" && printenv VAR1 && printenv VAR2 && echo "end" begin foo bar end $ echo $? 0 ``` As a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution) is invoked for each line of the recipe, there are no side effects of using `export`. It means this solution should not be considered invasive. Fixes bitcoin#22719. --- Also this PR removes no longer needed crutch from `qt.mk`. ACKs for top commit: fanquake: ACK affbf58 Tree-SHA512: 0ce2cf82870a7774bdf1592fac50857126ae47da902e349f1092d50109223be9d6a8efd5e92ec08c2ca775b17516482aabaf232378950ade36484a883acc177b
This PR makes possible to integrate other Qt modules (like QtQml or QtSvg) into our static builds, and fixes bug:
The current gitian binaries remains the same, e.g.: