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

[SCons] add AR config option (e.g. to set llvm-ar instead of ar) #1424

Merged
merged 1 commit into from Jan 21, 2023

Conversation

band-a-prend
Copy link
Contributor

@band-a-prend band-a-prend commented Jan 17, 2023

Changes proposed in this pull request

Some systems that are based on Clang/LLVM configurations could rely on llvm-ar archiver instead of ar to package static libraries. The proposed tiny patch offers additional config option (like CC, CXX) to allow user to set appropeate prefered AR variable.

Please refer Gentoo Linux issue: https://bugs.gentoo.org/773541

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

Closes # None

If applicable, provide an example illustrating new features this pull request is introducing

For build and tests of proposed feature the following common SCons configuration was used:

debug="no" 
f90_interface=y 
python_package=full 
python_cmd=python3.10 
optimize_flags="-Wno-inline" 
renamed_shared_libraries="no" 
use_pch="no" 
system_fmt="y" 
system_eigen="y" 
system_yamlcpp="y" 
env_vars="all" 
extra_inc_dirs="/usr/include/eigen3" 
prefix="/usr

The additional alternate options will be mentioned in appropreate cases.
The system Sundials 6.5.0 was used until test with sundials from Cantera submodule (Sundials 5.3.0)
The gfortran always is used to build f90_interface
The cantera.conf was removed before each build run!

Please notice the example of archiver call (ar or llvm-ar):

  1. Case: system_sundials="y" system_blas_lapack=n (lapack support is disabled)
ar rc build/lib/libcantera.a build/src/base/AnyMap.os ...

scons -j4 test
...
Tests passed: 4830
Up-to-date tests skipped: 0
Tests failed: 0
  1. Case: system_sundials="y" system_blas_lapack=y blas_lapack_libs="blas,lapack"
ar rc build/lib/libcantera.a build/src/base/AnyMap.os ...

scons -j4 test
...
Tests passed: 4830
Up-to-date tests skipped: 0
Tests failed: 0
  1. Case: AR=llvm-ar CC=clang CXX=clang++ system_sundials="y" system_blas_lapack=n (lapack support is disabled)
    ...
    AR = 'llvm-ar'
    CC = 'clang'
    CXX = 'clang++'
    ...

llvm-ar rc build/lib/libcantera.a build/src/base/AnyMap.os ...

scons -j4 test
...
Tests passed: 4830
Up-to-date tests skipped: 0
Tests failed: 0
  1. Case: AR=llvm-ar CC=clang CXX=clang++ system_sundials="y" system_blas_lapack=y blas_lapack_libs="blas,lapack"
    ...
    AR = 'llvm-ar'
    CC = 'clang'
    CXX = 'clang++'
    ...

llvm-ar rc build/lib/libcantera.a build/src/base/AnyMap.os ...

scons -j4 test
...
Tests passed: 4830
Up-to-date tests skipped: 0
Tests failed: 0
  1. Case: AR=llvm-ar CC=clang CXX=clang++ system_sundials="n" system_blas_lapack=y blas_lapack_libs="blas,lapack"
    ...
    AR = 'llvm-ar'
    CC = 'clang'
    CXX = 'clang++'
    ...
    system_sundials = 'n'
...
    CT_SUNDIALS_VERSION                 53
...

llvm-ar rc build/lib/libcantera.a build/ext/sundials/src/sundials/sundials_band.os ...

scons -j4 test
...
Tests passed: 4830
Up-to-date tests skipped: 0
Tests failed: 0

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

Signed-off-by: Sergey Torokhov <torokhov-s-a@yandex.ru>
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.

Proposed changes look good to me.

@codecov
Copy link

codecov bot commented Jan 19, 2023

Codecov Report

Merging #1424 (342521e) into main (1fa20b7) will decrease coverage by 0.26%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1424      +/-   ##
==========================================
- Coverage   70.84%   70.59%   -0.26%     
==========================================
  Files         376      376              
  Lines       54784    54781       -3     
  Branches    18137    18137              
==========================================
- Hits        38813    38671     -142     
- Misses      13506    13599      +93     
- Partials     2465     2511      +46     
Impacted Files Coverage Δ
include/cantera/kinetics/InterfaceKinetics.h 28.57% <0.00%> (-57.15%) ⬇️
include/cantera/kinetics/ReactionRateDelegator.h 71.42% <0.00%> (-19.05%) ⬇️
src/kinetics/Reaction.cpp 68.33% <0.00%> (-11.81%) ⬇️
src/kinetics/KineticsFactory.cpp 81.81% <0.00%> (-10.19%) ⬇️
...terfaces/dotnet/Cantera/src/Interop/InteropUtil.cs 60.75% <0.00%> (-8.87%) ⬇️
include/cantera/kinetics/InterfaceRate.h 81.92% <0.00%> (-6.03%) ⬇️
include/cantera/kinetics/TwoTempPlasmaRate.h 94.73% <0.00%> (-5.27%) ⬇️
include/cantera/kinetics/Falloff.h 78.22% <0.00%> (-3.23%) ⬇️
include/cantera/base/FactoryBase.h 68.75% <0.00%> (-3.13%) ⬇️
src/kinetics/Falloff.cpp 78.00% <0.00%> (-2.99%) ⬇️
... and 11 more

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

@ischoegl ischoegl merged commit 760b0fb into Cantera:main Jan 21, 2023
@band-a-prend
Copy link
Contributor Author

Thanks!

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