Skip to content

Conversation

dongcarl
Copy link
Contributor

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.

-----

Using AC_ARG_VAR we ensure the following properties:

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 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=... or make PATH=...

I believe that this makes using our depends system and our build system
much more consistent, and still allows overrides to be easily made by
those who need it.

@DrahtBot
Copy link
Contributor

Guix builds

File commit 78f912c
(master)
commit 6f67cd5
(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz f40de41106da4c97... 2115e8651ab18ace...
*-aarch64-linux-gnu.tar.gz dacf2940faee4f2c... ec1a4a0a4cbd9288...
*-arm-linux-gnueabihf-debug.tar.gz d8b533df92f3682c... 4b9b6fa06c219ea2...
*-arm-linux-gnueabihf.tar.gz bc972cd7e213059e... e726b01b3cad10d2...
*-riscv64-linux-gnu-debug.tar.gz ee4e485ca6befcf7... b3cd7511fce03105...
*-riscv64-linux-gnu.tar.gz 86a63852d986e861... 22c1dd944490b66c...
*-win-unsigned.tar.gz ca56f45f912430d5... 42bfc6ea747e7414...
*-win64-debug.zip 6d2c357bb63aed3b... f04754ad66a1f087...
*-win64-setup-unsigned.exe d24c1dbdbc9220e5... 50bf98fd9ffcd66d...
*-win64.zip 6cc3171c8e3c9c4c... 857e4a59844898c2...
*-x86_64-linux-gnu-debug.tar.gz 8bd6a31554f802c5... f8e1b7d54bcd74ea...
*-x86_64-linux-gnu.tar.gz 6817683ccfdde2e6... cfa1532d3abd518f...
*.tar.gz a48700f9f5d0158d... e9d8a2a90446ade8...
guix_build.log 23a0f0804c2d8e6a... 086cc84b7246273f...
guix_build.log.diff 15c03236698f96f2...

@DrahtBot
Copy link
Contributor

Gitian builds

File commit 055abfb
(master)
commit 66e0471
(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz 83b9a1fc8012b4f4... 440acac33d313b87...
*-aarch64-linux-gnu.tar.gz e8fd58acaf9ab533... bf1b584f296deb19...
*-arm-linux-gnueabihf-debug.tar.gz 36e69a3e0993c974... 237c7d6afc5a63ea...
*-arm-linux-gnueabihf.tar.gz 9d6c33bae12919b8... 6ca8b82cae1c0234...
*-osx-unsigned.dmg 9661ba77d7feadb6...
*-osx64.tar.gz e3321873788a6d44...
*-riscv64-linux-gnu-debug.tar.gz e8075e23e6d33111... 8ed44218faa3ef3d...
*-riscv64-linux-gnu.tar.gz 9bd820aca1c54d98... fa4b38514b827640...
*-win64-debug.zip f74bce7e95daa86c... 729ee90695f9f153...
*-win64-setup-unsigned.exe c73124f35a45d788... bfb263c29f0cd112...
*-win64.zip 23a4988b91d68620... 714ba9b67751c5a9...
*-x86_64-linux-gnu-debug.tar.gz 2db4e488319d7a43... 256fc74af9a07674...
*-x86_64-linux-gnu.tar.gz ec418549c3d8b9f8... d07e9bce1b78b77c...
*.tar.gz 1dd803b18c3edbcc... 79fc7d2ec37bd83c...
bitcoin-core-linux-0.21-res.yml e97e9d47892c6cd0... c310d468c9d4a210...
bitcoin-core-osx-0.21-res.yml 5fab350cbe1fa2e7...
bitcoin-core-win-0.21-res.yml 326c38449b1637b1... 87e011b8500abffd...
linux-build.log c9e863fc73dbbb6d... f81db22ff036c8e2...
osx-build.log e3eb454d139ca2a3... 65f8562334fb07b7...
win-build.log 1b2caca1a146f8a9... 91912fefb9942251...
bitcoin-core-linux-0.21-res.yml.diff 9154a584238d6782...
bitcoin-core-win-0.21-res.yml.diff 7fef9702e1dafc6d...
linux-build.log.diff b86516819e74b123...
osx-build.log.diff b870c0a4406342cc...
win-build.log.diff 299b937b4e0a701b...

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 28, 2020

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

Conflicts

Reviewers, 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.

@DrahtBot
Copy link
Contributor

Gitian builds

File commit c95784e
(master)
commit 238bd95
(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz af89af3798a0ab1a... a485899c5dd05787...
*-aarch64-linux-gnu.tar.gz 6f556b0a5258a2f3... 1938f30a5720c7f8...
*-arm-linux-gnueabihf-debug.tar.gz fe65378d17205db6... 8aa6f0f48032a35a...
*-arm-linux-gnueabihf.tar.gz dbd3a6b413eea102... 4bfe8e1c84cf150d...
*-osx-unsigned.dmg 0451246756507e53... 32e66b094e1ce449...
*-osx64.tar.gz b7288a690cb9c5fd... 361e0916cf0b7dee...
*-riscv64-linux-gnu-debug.tar.gz 635ebc3b72c227f3... 1b5870cf22c60608...
*-riscv64-linux-gnu.tar.gz 2422d3b2ca512f0d... 980d2c71de9f75c2...
*-win64-debug.zip f9c8fa8e18bca9ee... 37e740cd8df0b563...
*-win64-setup-unsigned.exe 0c68c5fd69c2035b... f9541db6c4973ecc...
*-win64.zip 2c6dbb588630b225... 91f30a2cf2a09ba5...
*-x86_64-linux-gnu-debug.tar.gz ff46d462b0abafe6... d71f56a68dea04c7...
*-x86_64-linux-gnu.tar.gz 7ba4a6fecc0fd5fe... 7ef85fa3a21762c9...
*.tar.gz fd7a65878cac98f0... 18604ad56d220b39...
bitcoin-core-linux-0.21-res.yml 0ce111bd3c34d633... fd0b632fca3534c7...
bitcoin-core-osx-0.21-res.yml 93cfb09dc520f034... 069c3a85bf34dc8f...
bitcoin-core-win-0.21-res.yml 620452093f99ab65... 8bb2d7c233381af1...
linux-build.log 0f5ac697891162aa... d122d8475b9f7dc6...
osx-build.log b44e26f7eaaf2a4d... 9c4ac5fc3ca10f3d...
win-build.log 6cd8e4e857291e95... 70e52806fa80b540...
bitcoin-core-linux-0.21-res.yml.diff 76403cbd0628e644...
bitcoin-core-osx-0.21-res.yml.diff 9f048522f830166d...
bitcoin-core-win-0.21-res.yml.diff e6571503cafb260c...
linux-build.log.diff 143f989f66619be6...
osx-build.log.diff 3241adf4dd90ea11...
win-build.log.diff 1a0ecc730d28aba1...

@fanquake
Copy link
Member

Concept ACK thing

@dongcarl
Copy link
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
@dongcarl dongcarl force-pushed the 2020-09-resolve-PATH-prepending-madness branch 4 times, most recently from 179a263 to d520d75 Compare November 13, 2020 17:57
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.
@dongcarl
Copy link
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.

@dongcarl dongcarl closed this Nov 23, 2020
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants