depends: Properly pass $PATH to configure and pin#20019
Closed
dongcarl wants to merge 8 commits intobitcoin:masterfrom
Closed
depends: Properly pass $PATH to configure and pin#20019dongcarl wants to merge 8 commits intobitcoin:masterfrom
dongcarl wants to merge 8 commits intobitcoin:masterfrom
Conversation
Contributor
dongcarl
commented
Sep 25, 2020
Contributor
Contributor
Gitian builds
|
Contributor
|
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. |
Contributor
Gitian builds
|
Member
|
Concept ACK thing |
Contributor
Author
|
Hehe, I've found that this issue is a little more complicated than what I've described in the original description, so keeping it as draft until I figure out a clean solution. |
Previously, if ./configure was invoked with:
```
$ env CONFIG_SITE=depends/x86_64-pc-linux-gnu/share/config.site ./configure
```
Where $CONFIG_SITE was a relative path, ./configure would fail with the
following misleading output:
```
checking for boostlib >= 1.58.0 (105800)... yes
checking whether the Boost::System library is available... yes
configure: error: Could not find a version of the Boost::System library!
```
Fully resolving depends_prefix in config.site.in fixes this. To make
sure that there are no other side effects I ran a diff on the
config.status generated by:
1. The scripts prior to this change with CONFIG_SITE set to a full path:
env CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure
2. The scripts after this change with CONFIG_SITE set to a relative path:
env CONFIG_SITE=depends/x86_64-pc-linux-gnu/share/config.site ./configure
And it looks good!
Diff: https://paste.sr.ht/~dongcarl/95b469fbc555c128046e85723d87a9082a754f6b
Files like config.site.in are not referenced by any other script in our tree, so we need to mark it manually with a "shellcheck shell=" directive and make sure that shellcheck is run on them.
Previously, when running ./configure: 1. With CONFIG_SITE pointed to our depends config.site.in, and 2. PYTHONPATH was not set either in the environment or by the user The configure would output something like: PYTHONPATH='depends/x86_64-pc-linux-gnu/share/../native/lib/python3/dist-packages:' When we really mean: PYTHONPATH='depends/x86_64-pc-linux-gnu/share/../native/lib/python3/dist-packages' ...without the colon This change makes sure that: 1. There's no trailing colon, and 2. We use the $PATH_SEPARATOR variable instead of a colon
179a263 to
d520d75
Compare
Previously, we did not persist the $PATH env var between bitcoin's
configure-phase and its make-phase.
This meant that when configuring with $CONFIG_SITE set to one generated
by depends, we would be configuring with $depends_prefix/native/bin
prepended to $PATH, as described in depends/config.site.in.
However, when make-ing, the $PATH would be set to whatever the caller's
environment is, which didn't include $depends_prefix/native/bin.
To work around this, we:
1. Prepended $depends_prefix/native/bin to our CC, CXX, AR, RANLIB,
NM, and STRIP in order to invoke them with a full path as argv[0].
2. Manually prepended $depends_prefix/native/bin to $PATH in our
Gitian/Guix builds.
These workarounds are not ideal, and there is a better way. Essentially
we want to make sure that what we feature-test at configure-time _is_
what we will run at make-time.
-----
In order to fix this, we introduce a new, non-precious ./configure
variable called $PATH_TEMPLATE, which is a shell snippet to be expanded
at ./configure-time to form $PATH.
Using PATH_TEMPLATE means that:
0. We no longer need the 2 workarounds described above. (They are
dropped in later commits)
1. By default and without any overrides, whatever $PATH is used at
configure-time, is whatever $PATH is at make-time.
2. When configuring with depends' CONFIG_SITE, the
$depends_prefix/native/bin prepend to $PATH stays there unless
overridden manually by a ./configure PATH_TEMPLATE=... or make
PATH_TEMPLATE=...
3. We have the flexibility to reference and compose configure variables,
which can be used for making our WRAP_DIR facilities work properly in
gitian.
For example,
PATH_TEMPLATE="$WRAP_DIR"':${depends_path_fragment}:${PATH}'
NOTE: I would recommend reviewers to look at the subsequent commits in
this patchset, which demonstrate the simplifications that this change
brings about.
-----
Example Scenarios:
Example 1: Plain configure (the default)
Invoking:
$ ./configure
Means that $PATH_TEMPLATE gets a default value of literally:
'${PATH}'
which is then expanded to form the PATH to be used for
the rest of ./configure and make
Example 2: Configure with depends CONFIG_SITE
Invoking:
$ env CONFIG_SITE=depends/.../share/config.site ./configure
Means that $PATH_TEMPLATE gets an alternate default value of
literally:
'${depends_path_fragment}${PATH_SEPARATOR}${PATH}'
which is then expanded to form the PATH to be used for the rest of
./configure and make
Example 3: Configure with custom PATH_TEMPLATE
Invoking:
$ WRAP_DIR=/home/satoshi/wrapped
$ env CONFIG_SITE=depends/.../share/config.site \
./configure PATH_TEMPLATE="$WRAP_DIR"':${depends_path_fragment}:${PATH}'
Means that both $depends_path_fragment and $PATH are left to be
expanded at configure-time since they are single-quoted while
$WRAP_DIR is expanded by the shell at invocation time. Therefore
$PATH_TEMPLATE gets a value of literally:
'/home/satoshi/wrapped:${depends_path_fragment}:${PATH}'
which is then expanded to form the PATH to be used for the rest of
./configure and make
See previous commit for a full description.
Since pointing to a CONFIG_SITE generated by our depends system will make ./configure prepend the appropriate depends $PATH fragment, we no longer need these now-redundant workarounds which tries to do the same prepending.
Use the new PATH_TEMPLATE configure variable to properly setup our $PATH, such that the search order at BOTH configure-time and build-time looks like: 1. The value of WRAP_DIR 2. The value of depends_path_fragment rest. The value of PATH This is ultimately what we wanted in the first case, but previously this was not achievable and led to many workarounds that can now be removed (like the GENISOIMAGE= configure variable specification removed in this commit).
It would seem that previously, because of the $PATH-related problems described in a previous commit in this patchset, the compression of the .iso file to a .dmg file was done outside of `make deploy' in order to use the faketime-wrapped version of libdmg-hfsplus's DMG tool. This is no longer a necessary workaround now that the underlying $PATH-related issues are solved.
d520d75 to
abfa657
Compare
This was referenced Nov 16, 2020
Contributor
Author
|
After another rethink, I was able to solve my original problems without this, so I'm going to close this PR for now. |
This file contains hidden or 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
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.