Update Script/Main.py so it uses importlib #3552
Merged
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
instead of deprecated imp module. import site_init.py file directly for Py3 Find spec for the site file by path rather than by fiddling sys.path to restrict the search. Py3: address the problem that we might get a "site_init.py" from somewhere else in sys.path. Clean up excess indentation / try nesting; adjust comment. Signed-off-by: Mats Wichmann <mats@linux.com>
Also simplify cleanup a bit - we don't need to leave the site file open, can use a context manager to read the code and let it close there. Signed-off-by: Mats Wichmann <mats@linux.com>
mwichmann
changed the title
3 tasks
bdbaddog
reviewed
Feb 14, 2020
| SCons.Tool.DefaultToolpath.insert(0, os.path.abspath(site_tools_dir)) | ||
|
|
||
| if not os.path.exists(site_init_file): |
There was a problem hiding this comment.
What's the impact of moving this below the site_tools_dir if above?
There was a problem hiding this comment.
just grouping, flow matches the reworded docstring.
Closed
bdbaddog
reviewed
Feb 19, 2020
| @@ -62,6 +62,7 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER | |||
| - Accommodate VS 2017 Express - it's got a more liberal license then VS | |||
| Community, so some people prefer it (from 2019, no more Express) | |||
| - vswhere call should also now work even if programs aren't on the C: drive. | |||
| - Script/Main.py now uses importlib instead of imp module. | |||
There was a problem hiding this comment.
Might be worth adding motivation.. imp deprecated and will be removed in Py#.#?
Are we still using imp elsewhere?
There was a problem hiding this comment.
If you want to insert the word "deprecated" when you decide to push it that's fine. I don't think there's a timeline, they're still dickering about deprecation (sounds familiar!).
Deprecated since version 3.4: The imp package is pending deprecation in favor of importlib.
If you want me to update it from here I will.
3 tasks
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.
Stop using the
impmodule in Main, replace withimportlib, for "importing" the site file. It started life as a side-by-side change with a Py3 version added to the existing code, but since originally being proposed (different PR), Python 2 support is being dropped, so it has been recast with just the importlib piece.This replaces PR #3399 (see some history there if interested). It relates to Issue #3299 .
Contributor Checklist:
master/src/CHANGES.txtdirectory (and read theREADME.txtin that directory)