Skip to content

Refactor SConstruct option handling / extract scons options v2#1137

Merged
bryanwweber merged 23 commits intoCantera:mainfrom
ischoegl:extract-scons-options-v2
Nov 22, 2021
Merged

Refactor SConstruct option handling / extract scons options v2#1137
bryanwweber merged 23 commits intoCantera:mainfrom
ischoegl:extract-scons-options-v2

Conversation

@ischoegl
Copy link
Copy Markdown
Member

@ischoegl ischoegl commented Nov 2, 2021 •
edited
Loading

Changes proposed in this pull request

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

Addresses Cantera/cantera-website#26

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

$ scons help --restructured-text
[...]
$ scons help --restructured-text --dev --output=config-options-dev.rst
[...]

or

$ scons help --restructured-text --option=prefix
scons: Reading SConscript files ...
INFO: SCons is using the following Python interpreter: /usr/bin/python3

.. _prefix:

*  ``prefix``: [ ``path/to/prefix`` ]
   Set this to the directory where Cantera should be installed. On Windows
   systems, ``$ProgramFiles`` typically refers to ``"C:\Program Files"``.

   -  default: platform dependent
      -  Windows: ``'$ProgramFiles\Cantera'``
      -  Otherwise: ``'/usr/local'``

Further

$ scons help --option=CXX
scons: Reading SConscript files ...
INFO: SCons is using the following Python interpreter: /usr/bin/python3

* CXX: [ string ]
    The C++ compiler to use.
    - default: '${CXX}'
    - actual: 'g++'

and

$ scons help --defaults --option=cc_flags
scons: Reading SConscript files ...
INFO: SCons is using the following Python interpreter: /usr/bin/python3

* cc_flags: [ string ]
    Compiler flags passed to both the C and C++ compilers, regardless of
    optimization level.
    - default: compiler dependent
      - If using MSVC: '/MD /nologo /D_SCL_SECURE_NO_WARNINGS /D_CRT_SECURE_NO_WARNINGS'
      - If using ICC: '-vec-report0 -diag-disable 1478'
      - If using Clang: '-fcolor-diagnostics'
      - Otherwise: ''

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 mentioned this pull request Nov 2, 2021
Closed
5 tasks
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Nov 2, 2021 •
edited
Loading

@bryanwweber ... I opened a new PR as the implementation did change a bit. I left the old buildutils.py:help in for the time being, but would replace it with a new version to avoid code redundancies.

@ischoegl ischoegl requested a review from bryanwweber November 2, 2021 22:47
@ischoegl ischoegl force-pushed the extract-scons-options-v2 branch 8 times, most recently from 0ecd549 to 5b957e2 Compare November 3, 2021 16:00
@ischoegl ischoegl force-pushed the extract-scons-options-v2 branch from 5b957e2 to eba57ae Compare November 3, 2021 16:34
@ischoegl ischoegl force-pushed the extract-scons-options-v2 branch from eba57ae to b0c0a2f Compare November 3, 2021 16:43
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Nov 3, 2021 •
edited
Loading

Never liked the command get-options in this context. I decided to make it part of scons help, which uses the option --restructured-text for ReST output. Also implemented help for an individual option, which works for both ReST and CLI.

@ischoegl ischoegl force-pushed the extract-scons-options-v2 branch from b88ee87 to 6f38147 Compare November 21, 2021 15:24
@ischoegl ischoegl force-pushed the extract-scons-options-v2 branch from 6f38147 to b1282c5 Compare November 21, 2021 16:08
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Nov 21, 2021 •
edited
Loading

@bryanwweber and @speth ... thanks for the reviews! I believe I took care of everything.

Also, thank you @bryanwweber for giving me a crash course on PEP 484. Although I've been using PEP 484 annotations for a while, I was apparently shockingly ignorant about some of the features. What is submitted now, passes Pylance's muster (no complaints on the added sections with basic type-hinting checks). Had to change a couple of things to avoid overly long type hints, but I think that the code quality benefited from those changes.

PS: ugh ... fixing PEP 484 broke the actual build 🤣 (which for whatever reason still works on my machine ... may be due to a pretty old version of scons I am using)

@ischoegl ischoegl force-pushed the extract-scons-options-v2 branch 3 times, most recently from d9a9aac to 5c8b938 Compare November 21, 2021 16:57
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Nov 21, 2021 •
edited
Loading

Looks like I was overly aggressive when removing Configuration.__iter__ 😂 ... it's useful when checking for the existence of keys as in if key in config: in SConstruct ... but things work now.

In addition, adding a couple of scons help commands to one of the CI runners (I picked Build docs) will prevent inadvertent breakages. I also added an upload of Build docs artifacts (commit 03b4243) just in case - those are kept for 2 days.

@ischoegl ischoegl marked this pull request as draft November 21, 2021 17:14
@ischoegl ischoegl force-pushed the extract-scons-options-v2 branch 2 times, most recently from 85b09c3 to 099c862 Compare November 21, 2021 17:33
@ischoegl ischoegl marked this pull request as ready for review November 21, 2021 17:34
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Nov 21, 2021 •
edited
Loading

🎉 ... I think this (finally?) takes care of everything. Found another instance of an issue with type hints (which percolated through a couple of things), but that's now taken care of. @bryanwweber ... all PEP 484 updates are squashed in 2ccda53.

As an aside, SCons and buildutils aren't resolved in SConstruct and raise a reportMissingImports flag in Pylance, causing a lot of 'Problems' to be reported. I don't think that I want to address this pre-existing condition in this PR 😂

PS: looks like I jinxed it - and had to force-push yet another time. At this point, I think it is done for real.

@ischoegl ischoegl force-pushed the extract-scons-options-v2 branch from 099c862 to b76e678 Compare November 21, 2021 21:08
@bryanwweber
Copy link
Copy Markdown
Member

bryanwweber commented Nov 21, 2021

As an aside, SCons and buildutils aren't resolved in SConstruct and raise a reportMissingImports flag in Pylance, causing a lot of 'Problems' to be reported. I don't think that I want to address this pre-existing condition in this PR 😂

Yeah, this is because SCons has a lot of "magic" things that are automatically imported for you when it runs. It basically has to be addressed at the Pylance level, as far as I can tell, since those things don't get imported in SConstruct/SConscript.

@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Nov 21, 2021

Yeah, this is because SCons has a lot of "magic" things that are automatically imported […]

That’s what I figured but I was hoping that there may be a clever way to account for this without having to tinker with whatever delinter/etc. someone may be using.

Regardless, looks like I learned a bit about PEP 484 in this PR … it was definitely worthwhile. In hindsight knowing about some details I know now would have made me avoid a couple of pitfalls. Fanciful helper functions are certainly something to stay away from, but it certainly makes for cleaner code. Also, enabling Pylance’s type hinting capabilities did clarify a couple of things.

That said, I think I’m happy with this as it stands … I think that apart from the reST, the added options for scons help should be beneficial down the line.

bryanwweber
bryanwweber approved these changes Nov 22, 2021
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.

Looks great @ischoegl! Thanks for the quick efforts to fix this up. It was likewise educational for me to look all this stuff up.

@bryanwweber bryanwweber merged commit c603ce3 into Cantera:main Nov 22, 2021
@ischoegl ischoegl deleted the extract-scons-options-v2 branch November 22, 2021 15:31
This was referenced Nov 22, 2021
Merged
Merged
@bryanwweber bryanwweber mentioned this pull request Nov 22, 2021
Merged
5 tasks
@ischoegl ischoegl mentioned this pull request Nov 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bryanwweber bryanwweber bryanwweber approved these changes

@speth speth Awaiting requested review from speth

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 @bryanwweber @speth