build: Improve depends build system robustness #22552
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
For the scripted-diff please use |
|
Updated fdaeb52 -> b48fd7b (pr22552.01 -> pr22552.02, diff):
|
|
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. |
jarolrod
reviewed
Jul 27, 2021
There was a problem hiding this comment.
Concept ACK
I agree with the vision this PR is trying to set. I wanted to try to document what this seems to be doing and aims to accomplish.
What this PR is doing:
We can notice some "fragility" in our build system when looking at the recent Android related issues and their patches. The OP shows a good example; somehow, the boost version ended up there. Another example is the patch set of #22555; it seems as though the wrong value is evaluated with specific variables.
Why might this be happening? This PR hits the nail on the head by attacking the use of recursive variables. Recursive variables make it quite hard to track where the value is coming from (to a build noob like me). The main change this PR is performing is refactoring from recursively expanded variables to simple expanded ones. This irons out any kinks that currently exist and seems to work for the Android build issues as reported here: #22555 (comment)
Per the make manual
"To avoid all the problems and inconveniences of recursively expanded variables, there is another flavor: simply expanded variables."
Should we do this?
Warning: The following opinions are from a build noob
We do not need to refactor to simply expanded variables. We could spend time figuring out what about our current setup and the use of recursive variables is contributing to the example in the OP and partially leading to other examples like #22522.
Nevertheless, we should move to simply expanded variables. The clear benefit is, again, stated in the make manual:
"Simply expanded variables generally make complicated makefile programming more predictable because they work like variables in most programming languages."
Performing this refactor makes future changes more accessible because we can easily track where values come from. Also, and while not a directly supporting reason for this change, it can lower the entry and learning curve barrier to contributing to our build system.
Also, simply expanded variables are newer. Newer = Better
Notes on commits:
- dc54e4a
- simlar to explanation for 2ee0cf3
- c705d1b
- This is mainly a cosmetic change. Not required
- 2ee0cf3
- If we are going to refactor to simple variables, this is needed to keep the proper order of evaluation. X.mk depends on native_X.mk.
- 096d24f
- This is a cosmetic change
- 4e70c20 & 6381331
- performs migration from recursive to simple variables
|
Guix Hashes, mine match @hebasto: |
|
Rebased 6381331 -> ad2a195 (pr22552.03 -> pr22552.04) due to the conflict with #22840. |
Thanks @jarolrod for your detailed explanation, it wasn't kind of clear to me in the OP what was exactly fragile here, and how it is fixed. I don't know a lot about makefile internals, but avoiding inadvertent issues that could result in wrong builds seems important. Concept ACK. Would be good if @theuni has a look over the changes I think. |
Closed
This was referenced Oct 12, 2021
|
Updated 17ef4c9 -> 4cba82d (pr22552.13 -> pr22552.14):
|
This was referenced Apr 23, 2022
|
Rebased 4cba82d -> 83f7c33 (pr22552.14 -> pr22552.15) due to the conflict with #25046. |
This was referenced Jun 15, 2022
-BEGIN VERIFY SCRIPT- FILES=$(git ls-files depends/packages) sed -i 's/package=/package := /' -- $FILES sed -i -e 's/$(package)\(\w\+\)=/$(package)\1 := /' -- $FILES -END VERIFY SCRIPT-
|
Rebased 83f7c33 -> df6dccd (pr22552.15 -> pr22552.16). |
This was referenced Jul 20, 2022
|
GUIX hashes: x86: arm64: |
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
|
Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened. |
fanquake
added a commit
to bitcoin-core/gui
that referenced
this pull request
Dec 8, 2022
… 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
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Dec 8, 2022
…`$(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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The depends build system looks fragile in its current state. For example, on master (346e780):
With this PR:
Required for #22555.