Skip to content

Disable Python 3.6#1222

Merged
ischoegl merged 5 commits intoCantera:mainfrom
ischoegl:warn-for-old-Python
Mar 23, 2022
Merged

Disable Python 3.6#1222
ischoegl merged 5 commits intoCantera:mainfrom
ischoegl:warn-for-old-Python

Conversation

@ischoegl
Copy link
Copy Markdown
Member

@ischoegl ischoegl commented Mar 20, 2022 •
edited
Loading

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 force-pushed the warn-for-old-Python branch from 024855a to fcb2a22 Compare March 20, 2022 14:33
@ischoegl ischoegl changed the title Warn when using Python 3.6 Disable Python 3.6 Mar 20, 2022
bryanwweber
bryanwweber reviewed Mar 20, 2022
View reviewed changes
Copy link
Copy Markdown
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.

@ischoegl ischoegl force-pushed the warn-for-old-Python branch from fcb2a22 to 4c13fe1 Compare March 20, 2022 15:07
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Mar 20, 2022 •
edited
Loading

Done!

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

ischoegl commented Mar 20, 2022 •
edited
Loading

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 ...

@ischoegl
[SCons] Always throw error if minimum Python version is not met
9a16cff 
Point users to option python_package='none' in error message.
@ischoegl ischoegl force-pushed the warn-for-old-Python branch from 67945a8 to 9a16cff Compare March 20, 2022 16:51
@ischoegl ischoegl requested a review from bryanwweber March 20, 2022 16:52
@speth
Copy link
Copy Markdown
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
Copy Markdown
Member

bryanwweber commented Mar 23, 2022

  • Check the Python version before importing buildutils in SConstruct

This would be my preference using sys.version

@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Mar 23, 2022

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
bryanwweber
bryanwweber reviewed Mar 23, 2022
View reviewed changes
speth
speth requested changes Mar 23, 2022
View reviewed changes
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 23, 2022 •
edited
Loading

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

@ischoegl ischoegl force-pushed the warn-for-old-Python branch from 790af21 to 1cc232e Compare March 23, 2022 20:17
@ischoegl ischoegl requested review from bryanwweber and speth March 23, 2022 20:17
speth
speth requested changes Mar 23, 2022
View reviewed changes
speth
speth approved these changes Mar 23, 2022
View reviewed changes
Copy link
Copy Markdown
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

Reviewers

@speth speth speth approved these changes

@bryanwweber bryanwweber bryanwweber approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ischoegl @speth @bryanwweber