-
Notifications
You must be signed in to change notification settings - Fork 37.7k
depends: Properly pass $PATH to configure and pin #20019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
depends: Properly pass $PATH to configure and pin #20019
Conversation
dongcarl
commented
Sep 25, 2020
Gitian builds
|
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. |
Gitian builds
|
Concept ACK thing |
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
After another rethink, I was able to solve my original problems without this, so I'm going to close this PR for now. |