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

Disable Python 3.6 #1222

Merged
merged 5 commits into from
Mar 23, 2022
Merged

Disable Python 3.6 #1222

merged 5 commits into from
Mar 23, 2022

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Mar 20, 2022

Changes proposed in this pull request

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

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

@ischoegl ischoegl changed the title Warn when using Python 3.6 Disable Python 3.6 Mar 20, 2022
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.

I think we should have a check at the top of SConstruct as well that the Python running SCons is >=3.7. But that can wait until after 2.6, I suppose, since it would preclude system Pythons from even running SCons and we're not using (nor will we add any, I bet) 3.7-specific features into the various SCons* files before 2.6 is released.

SConstruct Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

ischoegl commented Mar 20, 2022

Done!

@ischoegl ischoegl mentioned this pull request Mar 20, 2022
5 tasks
@ischoegl
Copy link
Member Author

ischoegl commented Mar 20, 2022

I wonder if it wouldn't be preferable to error out for Python 3.6 if python_package = 'default', and indicate that it can still be installed for python_package = 'none'. Going through the entire build/installation process only to realize that Python was automatically skipped is not ideal ...

Point users to option python_package='none' in error message.
@speth
Copy link
Member

speth commented Mar 23, 2022

As a result of #1158, the requirement of Python 3.7 or newer applies to the Python interpreter running scons. If you try to build using Python 3.6, you will get the error:

scons: Reading SConscript files ...
  File "/src/cantera/site_scons/buildutils.py", line 1

    from __future__ import annotations
                                     ^

SyntaxError: future feature annotations is not defined

I don't think you'll ever get to the error handling added here except in the somewhat unlikely case where scons is running using Python 3.7+ but python_cmd has been explicitly pointed to an older Python installation.

The two options I see for providing a more helpful are either:

  • Put an end of line comment after from __future__ import annotations.
  • Check the Python version before importing buildutils in SConstruct

@bryanwweber
Copy link
Member

  • Check the Python version before importing buildutils in SConstruct

This would be my preference using sys.version

@ischoegl
Copy link
Member Author

Check the Python version before importing buildutils in SConstruct

Np. What I had worked well prior to merging #1158. I added the check as requested, but left the version decision separate from the pre-existing checks.

@ischoegl ischoegl force-pushed the warn-for-old-Python branch 2 times, most recently from 1a2aaf5 to 790af21 Compare March 23, 2022 19:34
SConstruct Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #1222 (790af21) into main (4bfd2a8) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 790af21 differs from pull request most recent head 1cc232e. Consider uploading reports for the commit 1cc232e to get more accurate results

@@            Coverage Diff             @@
##             main    #1222      +/-   ##
==========================================
+ Coverage   65.44%   65.47%   +0.03%     
==========================================
  Files         320      327       +7     
  Lines       46321    46349      +28     
  Branches    19688    19688              
==========================================
+ Hits        30315    30349      +34     
  Misses      13475    13475              
+ Partials     2531     2525       -6     
Impacted Files Coverage Δ
src/kinetics/RxnRates.cpp 78.26% <0.00%> (-14.47%) ⬇️
include/cantera/kinetics/ReactionData.h 76.00% <0.00%> (-8.22%) ⬇️
src/kinetics/Arrhenius.cpp 95.34% <0.00%> (-3.65%) ⬇️
include/cantera/kinetics/Falloff.h 74.57% <0.00%> (-3.41%) ⬇️
include/cantera/kinetics/RxnRates.h 90.90% <0.00%> (-2.43%) ⬇️
include/cantera/kinetics/ReactionRate.h 84.09% <0.00%> (-2.28%) ⬇️
src/kinetics/InterfaceRate.cpp 91.07% <0.00%> (-1.42%) ⬇️
include/cantera/kinetics/InterfaceRate.h 84.33% <0.00%> (-1.20%) ⬇️
src/kinetics/Reaction.cpp 81.14% <0.00%> (-0.18%) ⬇️
src/zeroD/Reactor.cpp 81.31% <0.00%> (ø)
... and 25 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

SConstruct Outdated Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, @ischoegl. Feel free to merge once the CI passes.

@ischoegl ischoegl merged commit 10ba578 into Cantera:main Mar 23, 2022
@ischoegl ischoegl deleted the warn-for-old-Python branch March 24, 2022 02:05
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.

3 participants