Skip to content
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

Allow alternate bootstrap toolset #137

Closed
wants to merge 3 commits into from

Conversation

jeking3
Copy link
Collaborator

@jeking3 jeking3 commented Feb 5, 2022

  • The bootstrap for the windows gcc task was using msvc see: Setup Boost step
  • Added B2_BOOTSTRAP_TOOLSET to allow expressing an alternate toolset for building boost build.

This closes #138

@jeking3 jeking3 added the bug Something isn't working label Feb 5, 2022
@jeking3 jeking3 self-assigned this Feb 5, 2022
@codecov
Copy link

codecov bot commented Feb 5, 2022

Codecov Report

Merging #137 (7b4b757) into master (0e2c610) will increase coverage by 15.38%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           master      #137       +/-   ##
============================================
+ Coverage   84.61%   100.00%   +15.38%     
============================================
  Files           2         2               
  Lines          13        16        +3     
  Branches        0         7        +7     
============================================
+ Hits           11        16        +5     
+ Misses          2         0        -2     
Impacted Files Coverage Δ
test/test.cpp 100.00% <0.00%> (+12.50%) ⬆️
include/boost/boost-ci/boost_ci.hpp 100.00% <0.00%> (+20.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e2c610...7b4b757. Read the comment docs.

@jeking3 jeking3 changed the title fix up running mingw build on windows Fix running bootstrap with the wrong toolset Feb 5, 2022
@jeking3 jeking3 force-pushed the ci/fix-gha-mingw-windows-bootstrap branch 5 times, most recently from 6faf7b1 to b90bf4c Compare February 6, 2022 01:20
@Flamefire
Copy link
Collaborator

Flamefire commented Feb 6, 2022

* The bootstrap for the windows gcc task was using msvc [see: Setup Boost step](https://github.com/boostorg/boost-ci/runs/5071617982?check_suite_focus=true)

* The b2 headers task was not setting the toolset (not particularly important, but it was identifying msvc not gcc)

* bootstrap on clang jobs was using gcc.

Let's discuss this in #138: In short: IMO bootstrap can use any compiler and there is no need to force the use of the test compiler, but it could be useful to let library authors set a specific bootstrap toolset when required. I.e. use B2_BOOTSTRAP_TOOLSET as toolset if set else do not pass any toolset.

* The path to mingw64 was not set up properly; as github documentation states the msys64 directory is not placed into the runner's default PATH.

Not fully sure here. The docu lists "Mingw-w64 8.1.0" and even without a PATH change a GCC-8 is found by the current jobs: https://github.com/boostorg/boost-ci/runs/5071617982?check_suite_focus=true
I think Msys64 and ingw refer to different things. So the PATH change is indeed not required as Mingw is already in the PATH? I remotely remember that some time ago I set up PATH to Msys64 to use that bash and not the git bash, but can't remember the details or find it.

This can replace the appveyor mingw64 task; we can probably from the mingw32 task at this point.

We do have redundant jobs on the different CIs already which IMO is a good thing as the environment might be slightly different and hence bugs may be caught with higher probability. Also if one CI isn't used (or can't be used anymore, see e.g. Travis) we have a fallback.

@Flamefire Flamefire added annoyance Not strictly a bug, but may be a problem and removed bug Something isn't working labels Feb 6, 2022
@jeking3
Copy link
Collaborator Author

jeking3 commented Feb 6, 2022

Yes, it was picking up the compiler from a chocolatey directory, and it probably still will. I have trimmed this down just to providing the B2_BOOTSTRAP_TOOLSET option.

@jeking3 jeking3 force-pushed the ci/fix-gha-mingw-windows-bootstrap branch from b90bf4c to 6a1a7bf Compare February 6, 2022 13:37
@jeking3 jeking3 changed the title Fix running bootstrap with the wrong toolset Allow alternate bootstrap toolset Feb 6, 2022
Copy link
Collaborator

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add the (currently unused) matrix feature to the posix part as well, see the inline comment. Just so library authors can easily use it the same as you did in the Windows part. No need to add an element to the matrix there as the windows job serves as an example already on how to do it if anyone wants to.

The 2nd is minor I think. The current bootstrap seems to treat an empty --with-toolset= the same as when not passed, but I'd rather be careful and the suggested change should make it clear, that it is optional. Speaking about that: Maybe add a comment to the top of the common_install.* files that this variable is optional and usually not required as the test and bootstrap toolsets can explicitly be different? Basically a summary of our discussion in case anyone wonders in the future.

ci/common_install.sh Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@jeking3 jeking3 force-pushed the ci/fix-gha-mingw-windows-bootstrap branch from a2bbf67 to 8f4534b Compare February 7, 2022 12:14
Copy link
Collaborator

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was picking up the compiler from a chocolatey directory, and it probably still will.

Oh interesting. I haven't found any other mention of a GCC in the software docu for the runners. Anyway per the way of least effort: Let's just keep it. We don't say we use mingw, the matrix says toolset: gcc and we get a GCC 🤷 I mean for MinGW/Cygwin we have other jobs (on appveyor) or can extend these configs if we wanted to. For now I'd say "any gcc" is good enough ;-)

Added a suggestion for a better wording of the comment. IMO it is important to note that a) what happens when unset and b) that it doesn't need to be the same. Yes both is there implicitly but I'd say it very explicitly to avoid confusion.

Also note that currently this doesn't seem to be working. For the posix jobs B2_BOOTSTRAP_TOOLSET would need to be set by lib authors in a non-obvious way (hence my suggestion to set it from the matrix in the posix jobs even if that is currently unused) and for mingw it currently does nothing: https://github.com/boostorg/boost-ci/runs/5092859106?check_suite_focus=true

D:\a\boost-ci\boost-root>cmd /c bootstrap mingw 
Building Boost.Build engine
...
###
### Using 'vc143' toolset.

So invokes bootstrap with "mingw" but still uses vc??? TBH I don't know why, it works locally for me.
Maybe it needs quoting? cmd /c "bootstrap %B2_BOOTSTRAP_TOOLSET%"

I'm really wondering if this is worth the effort... :-(

ci/common_install.bat Outdated Show resolved Hide resolved
ci/common_install.sh Outdated Show resolved Hide resolved
@jeking3 jeking3 force-pushed the ci/fix-gha-mingw-windows-bootstrap branch from 8f4534b to 482c5a8 Compare February 7, 2022 16:18
jeking3 and others added 2 commits February 7, 2022 11:19
Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
@jeking3
Copy link
Collaborator Author

jeking3 commented Feb 7, 2022

I'm really wondering if this is worth the effort... :-(

I think so, especially with it boiled down to what it is now, but up to you. Don't want it, I will delete it.
In my testing using mingw resulted in gcc being used to build b2. If that's not happening I would be surprised, but it's possible :)

@Flamefire
Copy link
Collaborator

I'm really wondering if this is worth the effort... :-(

I think so, especially with it boiled down to what it is now, but up to you. Don't want it, I will delete it. In my testing using mingw resulted in gcc being used to build b2. If that's not happening I would be surprised, but it's possible :)

I just don't want you to waste your time as this seems to cause unknown issues which seem hard to reproduce and to fix. I.e. the Windows GHA job with bootstrap mingw still uses MSVC: https://github.com/boostorg/boost-ci/runs/5096251220?check_suite_focus=true

And as neither me nor you can reproduce that locally I would just leave it as-is (i.e. with "any compiler for bootstrap") instead of trying to figure out what happens here. At least until someone actually needs to use a different bootstrap toolset.
Maybe keep the branch for that time so we have a good starting point.

Of course if you want to figure this out and manage to make it work I'm happy to approve/merge this (given that it doesn't change the default behavior or otherwise influences currently working jobs)

@jeking3
Copy link
Collaborator Author

jeking3 commented Feb 9, 2022

Yeah, sounds good to me... it probably needs the c:\mingw64\usr\bin path I was adding before.

@jeking3 jeking3 closed this Feb 9, 2022
@jeking3 jeking3 deleted the ci/fix-gha-mingw-windows-bootstrap branch February 9, 2022 02:26
@Flamefire
Copy link
Collaborator

Well if I run that locally it tries to use GCC right away and fails as it cannot find it as it should. So no idea what is different on GHA CI. Anyway: Postponed to later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
annoyance Not strictly a bug, but may be a problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bootstrap is not invoked with a toolset so it uses "whatever"
2 participants