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

Refactor SConstruct option handling / extract scons options v2 #1137

Merged
merged 23 commits into from
Nov 22, 2021

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Nov 2, 2021

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
Copy link
Member Author

ischoegl commented Nov 2, 2021

@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 force-pushed the extract-scons-options-v2 branch 8 times, most recently from 0ecd549 to 5b957e2 Compare November 3, 2021 16:00
@ischoegl
Copy link
Member Author

ischoegl commented Nov 3, 2021

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
Copy link
Member Author

ischoegl commented Nov 21, 2021

@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
Member Author

ischoegl commented Nov 21, 2021

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
Member Author

ischoegl commented Nov 21, 2021

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

@bryanwweber
Copy link
Member

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
Member Author

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.

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.

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

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