[SCons] Fix stage directory from appearing in setup_cantera #1102
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bryanwweber
requested changes
Sep 20, 2021
interfaces/cython/SConscript
Outdated
| @@ -122,6 +123,10 @@ else: | |||
| # Install Python module in the default location | |||
| extra = '' | |||
|
|
|||
| if env['stage_dir']: | |||
| # Get the absolute path to the stage directory | |||
| extra += ' --root=' + str(Path.cwd().parents[1].joinpath(env["stage_dir"])) | |||
There was a problem hiding this comment.
This line is long enough to me to deserve its own variable, so it can clarify what is happening here. In particular, an example of what Path.cwd().parents[1] should be, in a comment, would help future us remember why this is here. I have a couple of questions as well.
- Is
Path.cwd()always guaranteed to be the directory that we want? I mean,Path.cwd()returns the current working directory of the Python process, which is not necessarily the root of the project. I think it should be, because SCons requires the SConstruct file, but is there a more stable way to find the right path? stage_dirmay have relative path components in it, so probably best to.resolve()the path after joining.- You can avoid the
str(...)by using an f-string here
There was a problem hiding this comment.
Regarding (1), SCons changes the working directory such that cwd() here is the directory containing the SConscript file, the interfaces/cython subdirectory of the Cantera source directory. I think that can be regarded as stable behavior.
Fixes setup_cantera and the post-install message to reference the path of the Python module on the target system rather than the staging directory. Also, generated .pyc files no long specify the path in the staging directory. This has no effect on the use of the .pyc files, but helps avoid warnings/errors from packaging system linters (notably, on FreeBSD). Fixes Cantera#1094
bryanwweber
approved these changes
Sep 20, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes proposed in this pull request
python_module_locto remove the stage directory prefix so that doesn't show up in thesetup_canterascript or the post-install message..pycfiles.On this second point, I'm hoping we're finally using the
--rootoption tosetuptoolscorrectly. Previously (#296) we had tried using it in some cases and not been able to get it to do what we wanted.If applicable, fill in the issue number this pull request is fixing
Fixes #1094
If applicable, provide an example illustrating new features this pull request is introducing
Checklist
scons build&scons test) and unit tests address code coverage