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

Miscellaneous build system improvements, and mark release of Cantera 2.5.0 #974

Merged
merged 9 commits into from
Feb 8, 2021

Conversation

speth
Copy link
Member

@speth speth commented Feb 7, 2021

Most of these changes address issues I ran into while trying to build the Ubuntu package. A couple are related to issues identified while investigating #969.

Changes proposed in this pull request

  • Update shebang lines in Python scripts to specify python3
  • Add man pages for the CTML converter scripts
  • Make SRI test data file non-executable
  • Remove warning about untested Sundials versions for versions up to 5.7 (which is the current release)
  • Print the installation location of the minimal Python module
  • Update the logic for installing the minimal Python module to match what we do for the full module
  • Don't invite the use of sudo when running scons install (per discussion on Compiling Cantera from source fails to find numpy and cython in conda environment #963)

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

Fixes #969

Checklist

  • There is a clear use-case for this code change
  • The commit message has a short title & references relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • The pull request is ready for review

@speth speth changed the title Miscellaneous build system improvements Miscellaneous build system improvements, and mark release of Cantera 2.5.0 Feb 7, 2021
@speth speth marked this pull request as ready for review February 7, 2021 21:32
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 @speth! I have a few questions/comments:

  1. I don't understand the commit message on e590fb6. I don't see how that change will affect only the minimal interface. Can you clarify the commit message?
  2. I checked the conversion scripts to make sure any changes since a3 would not change the output. I found that air.cti had a little bit of reduced precision compared to gri30_tran.dat, meaning that there's an extra digit of precision in the new air.yaml compared to air.cti. I doubt this will affect anyone's results too much, but we probably want to note it in the release notes somewhere.
  3. The ohmech.cti file has the phase ohmech-multi, which should be added to ohmech.yaml and deprecated, like we did for gri30.yaml.

Otherwise, I think these changes look good!

@speth
Copy link
Member Author

speth commented Feb 8, 2021

e590fb6 only affects the minimal Python module because it's editing interfaces/python_minimal/SConscript, not interfaces/cython/SConscript.

@bryanwweber
Copy link
Member

e590fb6 only affects the minimal Python module because it's editing interfaces/python_minimal/SConscript, not interfaces/cython/SConscript.

🤦‍♂️ Dangit! Thank you for pointing that out.

@speth
Copy link
Member Author

speth commented Feb 8, 2021

Regarding (2), the issue with transport coefficient precision dates from the old C++ version of ck2cti. I fixed the values in gri30*.cti in 8bcb62f, which is part of this release, but had not noticed that it affected any other CTI files. I guess we will have the corrected coefficients for any YAML input files that were created using ck2yaml rather than cti2yaml. And agreed that this should be noted in the changelog, as the change for gri30.cti did slightly change a bunch of test results.

Matches the definition in h2o2.cti. Like what we are doing with the
gri30.cti -> gri30.yaml transition, this phase name is immediately
deprecated.
Update 'cantera-version' in all YAML files to correspond to the stable
release.
@speth
Copy link
Member Author

speth commented Feb 8, 2021

@bryanwweber, I added commit fb98298 which I think takes care of (3).

@bryanwweber
Copy link
Member

Woo! Feel free to merge + tag when the CI finishes.

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.

Error starting the program
2 participants