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

Fixes to help with conda-forge builds #1591

Merged
merged 12 commits into from Aug 21, 2023
Merged

Conversation

speth
Copy link
Member

@speth speth commented Aug 17, 2023

Changes proposed in this pull request

Fixes to resolve issues encountered while trying to update the conda-forge recipe (see conda-forge/cantera-feedstock#27), plus (unrelated) removal of a few unnecessary declarations.

  • Relax tolerance on test_phase_order_surf_jacobian, which was failing by a slim margin on ppc64le Linux builds
  • Fix OpenMP flags in generated CMakeLists.txt files -- see https://gitlab.kitware.com/cmake/community/-/wikis/doc/cmake/VariablesListsStrings for some help understanding the rather non-intuitive CMake syntax.
  • Implement workaround in sample SConstruct files for SCons ignoring RPATH on macOS
  • Improve setting compiler in sample SConstruct files: do not set a default compiler for "package" builds, and allow the user to override using standard environment variables such as CXX
  • Always allow the conda layout when using package_build
  • Check return value of soln_newSolution in clib demo to avoid undefined behavior if the input file is not found
  • Search the active conda environment for YAML files
  • Fix cleanup of -isysroot flags in package builds
  • Remove declaration for the method MultiTransport::correctBinDiffCoeffs which had no corresponding implementation
  • Remove friend declarations from DenseMatrix -- these methods either don't exist with the specified signature (invert) or only access public members of the DenseMatrix class (solve) and don't need to be friends.

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

The two values need to be combined as a single string to prevent CMake
from interpreting them as a list and inserting a semicolon that results
in a broken compiler command that looks like this:

    g++ -I/some/path;-fopenmp source_file.cpp ...
This test was failing by a slim margin on ppc64le Linux builds
@speth speth added compiling tests Issues about tests, running the tests, or test results labels Aug 17, 2023
@speth speth marked this pull request as draft August 17, 2023 02:58
@ischoegl
Copy link
Member

ischoegl commented Aug 17, 2023

There is another tolerance issue in #1271 (which was resolved by patching downstream) - I'd consider adopting this for the same rationale as provided here for conda-forge?

@speth
Copy link
Member Author

speth commented Aug 17, 2023

I stand by my comment in that PR, and don't think the patch proposed there should be adopted as our baseline. It's not even clear at this point whether that patch is still necessary on those systems, or whether it was just a transient due to specific builds of g++ and/or glibc.

@ischoegl
Copy link
Member

ischoegl commented Aug 17, 2023

I stand by my comment in that PR, and don't think the patch proposed there should be adopted as our baseline. It's not even clear at this point whether that patch is still necessary on those systems, or whether it was just a transient due to specific builds of g++ and/or glibc.

Sure. I was mainly asking as the tolerance adjustment was pretty minor, going from rtol = 1e-14; to rtol = 1.2e-14; - see proposed change. I'm fine either way, but I thought I'd bring it up. PS: It is true that it is no longer clear whether this is needed any longer, and it probably shouldn't change under those circumstances.

@speth speth marked this pull request as ready for review August 21, 2023 00:50
@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #1591 (c85fdb5) into main (cde1e79) will decrease coverage by 0.03%.
Report is 2 commits behind head on main.
The diff coverage is 16.66%.

❗ Current head c85fdb5 differs from pull request most recent head 2ba2cb0. Consider uploading reports for the commit 2ba2cb0 to get more accurate results

@@            Coverage Diff             @@
##             main    #1591      +/-   ##
==========================================
- Coverage   70.61%   70.58%   -0.03%     
==========================================
  Files         379      379              
  Lines       59153    59169      +16     
  Branches    21252    21257       +5     
==========================================
- Hits        41768    41762       -6     
- Misses      14311    14331      +20     
- Partials     3074     3076       +2     
Files Changed Coverage Δ
include/cantera/numerics/DenseMatrix.h 100.00% <ø> (ø)
include/cantera/transport/MultiTransport.h 50.00% <ø> (ø)
samples/clib/demo.c 77.77% <0.00%> (-19.90%) ⬇️
src/base/application.cpp 59.51% <42.85%> (-0.99%) ⬇️

... and 1 file with indirect coverage changes

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

@speth speth marked this pull request as draft August 21, 2023 02:35
@speth speth marked this pull request as ready for review August 21, 2023 23:15
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Thanks, @speth ... this looks all good to me!

@ischoegl ischoegl merged commit 8e2ab16 into Cantera:main Aug 21, 2023
39 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiling tests Issues about tests, running the tests, or test results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants