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

Integrate SCons 4.4.0 MSVC options #1404

Merged
merged 3 commits into from
Nov 4, 2022
Merged

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Nov 3, 2022

Changes proposed in this pull request

  • introduce msvc_toolset_version option

If applicable, fill in the issue number this pull request is fixing

Supersedes #1392

Addresses SCons/scons#3664

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Merging #1404 (78b9731) into main (f6744ec) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1404      +/-   ##
==========================================
+ Coverage   71.03%   71.06%   +0.03%     
==========================================
  Files         363      363              
  Lines       51855    51911      +56     
  Branches    17362    17374      +12     
==========================================
+ Hits        36834    36890      +56     
+ Misses      12676    12674       -2     
- Partials     2345     2347       +2     
Impacted Files Coverage Δ
src/oneD/IonFlow.cpp 50.61% <0.00%> (-0.61%) ⬇️
src/zeroD/IdealGasMoleReactor.cpp 92.30% <0.00%> (-0.55%) ⬇️
src/zeroD/IdealGasConstPressureMoleReactor.cpp 73.22% <0.00%> (-0.38%) ⬇️
src/zeroD/Reactor.cpp 86.50% <0.00%> (-0.23%) ⬇️
src/equil/vcs_solve_TP.cpp 61.65% <0.00%> (-0.05%) ⬇️
src/equil/vcs_solve.cpp 66.14% <0.00%> (-0.04%) ⬇️
interfaces/cython/cantera/onedim.py 88.22% <0.00%> (-0.02%) ⬇️
include/cantera/oneD/StFlow.h 94.64% <0.00%> (ø)
interfaces/cython/cantera/_onedim.pyx 79.40% <0.00%> (+0.42%) ⬆️
src/oneD/Boundary1D.cpp 54.40% <0.00%> (+0.55%) ⬆️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ischoegl
Copy link
Member Author

ischoegl commented Nov 3, 2022

@bryanwweber .. so this runs through (caveat: unrelated upstream macOS issues, which will probably resolve themselves): there was only a trivial glitch left in #1392 (simple key error).

Using both SCons 4.3.0 and 4.4.0, I tried messing with the tool set versions on my local MSVC installation. Kicking out 14.2 from installed MSVC tools makes the version unavailable (I no longer have VS 2019). Tests show that I can still use the 14.2 toolset for the 14.3 version, which I assume to be the correct behavior.

@ischoegl ischoegl marked this pull request as ready for review November 3, 2022 14:51
@ischoegl ischoegl requested a review from a team November 3, 2022 14:51
Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Thanks for taking this! Just one question.

.github/workflows/main.yml Outdated Show resolved Hide resolved
@bryanwweber
Copy link
Member

Any idea about the Mac os failures?

@ischoegl
Copy link
Member Author

ischoegl commented Nov 3, 2022

Any idea about the Mac os failures?

Not really. Things compile fine on my M2 ... I think it's a temporary upstream issue with the toolchain?

Loading library to get build settings and version: libhdf5.dylib
      error: Unable to load dependency HDF5, make sure HDF5 is installed properly
      error: dlopen(libhdf5.dylib, 6): image not found

@bryanwweber
Copy link
Member

Yeah that's a very strange error. I'm hesitant to merge with those failing though.

@ischoegl
Copy link
Member Author

ischoegl commented Nov 4, 2022

Yeah that's a very strange error. I'm hesitant to merge with those failing though.

We have the same failure on #1401 (merged - see https://github.com/Cantera/cantera/actions/runs/3390365384) so it’s definitely not due to anything we’re doing here.

@bryanwweber
Copy link
Member

I can't merge for some reason, some authentication thing on my phone. Please go ahead!

@ischoegl ischoegl merged commit 577455c into Cantera:main Nov 4, 2022
@ischoegl ischoegl deleted the scons-msvc branch November 28, 2022 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants