-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
[SCons] Implement script to extract options from SConstruct #1136
Conversation
1fff9c0
to
15690ce
Compare
58769ea
to
ef61487
Compare
Done. For output generated by the script, refer to Cantera/cantera-website#160. As this PR stands, the parsed options are uploaded together with documentation into a new |
bd1b4db
to
8f4a299
Compare
9e05d9f
to
bba50ea
Compare
bba50ea
to
91e0db8
Compare
Noticed a couple of additional manual edits in the original |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ischoegl Reworking the defaults
class is certainly welcome and a nice cleanup. I do have a small concern that everything is equivalent for the compilers that we don't run on GH Actions (Cygwin, mingw, Intel), since some of that stuff broke when I updated buildutils.py
. Is there any way we can address that?
'Set this to the directory where Cantera should be installed.', | ||
"prefix", | ||
"""Set this to the directory where Cantera should be installed. On Windows | ||
systems, '$ProgramFiles' typically refers to "C:\Program Files".""", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify what "typically" means here? If a user doesn't know what '$ProgramFiles'
is or what it does, how should they interpret this help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should become clear from the context once the ReST is assembled ... the corresponding portion is
.. _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'``
The original (hand-coded) ReST explicitly referred to C:\Program Files\Cantera
, but that is not what SConstruct
actually implements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, the ReST looks good, but does scons help
return all that context too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point taken!
'', PathVariable.PathAccept), | ||
EnumVariable( | ||
'lapack_names', | ||
"""Set depending on whether the procedure names in the specified | ||
libraries are lowercase or uppercase. If you don't know, run 'nm' on | ||
the library file (for example, 'nm libblas.a').""", | ||
the library file (for example, "nm libblas.a").""", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be OK to leave as '
since it's representing a program/CLI thingy, and that's our convention elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's unfortunately a little tricky, as it's very hard to guess what single quotes enclose (for example, you'd have to catch exceptions for all kinds of contractions "isn't", "doesn't", "can't", "you'd", "that's"). My way around this was to limit single quote extents to whatever doesn't include whitespace. So using double quotes here is imho the easier solution (and it is still clear from a CLI perspective).
@bryanwweber ... thanks for the comments!
Not sure - what I did here is to map the current logic of PS: It is correct that this assumes that |
Yes, I know, but in any refactoring there's a chance of introducing bugs or incidentally changing behavior, I just hope that doesn't happen here. I guess the resolution is just Cantera/enhancements#117.
That's a good idea, I wonder where that should go... probably in the compiling docs on the website would be a good spot. I'm going to move the larger conversation about the home for this functionality out here into the main thread, so hopefully it doesn't get collapsed by GitHub quite so quickly... I never said, thank you @ischoegl for taking on the first implementation of this! Clearly that issue has been around for almost 3 years, so it's nice to finally have something concrete to discuss. So, on looking through the I think it would be considerably less fragile to have a new SCons option (like Like Once that new option is added to SConstruct here, we can add a plugin to the website repo to run |
@bryanwweber ... thanks for the additional feedback. My initial reaction is that I am not sure that I agree with that assessment. The nice thing about AST is that you know what the original expressions are that are plugged into other expressions, while leaving things in plain code. The alternative would be to build a mechanism that detects options for Either way, much of the work that is already done can be leveraged - the |
201b880
to
71ebed1
Compare
If I understand your concern correctly, this is already done by SCons when |
That's not exactly what happens for platform/compiler-dependent options, as the default option is not known a priori. The way around this that I can see is to implement wrappers, that can generate SCons variables, but also output ReST. (e.g. wrap It's much easier to resolve for the |
Overall, I'm 👍 with adding |
0adfc6c
to
2e1ffd1
Compare
2e1ffd1
to
54e2600
Compare
Superseded by #1137 |
Changes proposed in this pull request
scons2rst.py
, which extracts options fromSConstruct
via ASTSConstruct
in order to provide access to platform/compiler dependent defaultsSConstruct
options (including platform/compiler dependent defaults) are extracted and written to a ReST fileIf 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
An example for generated output for the development branch is config-options.rst.txt
At the moment, platform-dependent options are described as
<platform dependent>
.Checklist
scons build
&scons test
) and unit tests address code coverage