Replace use of imp library with importlib. #3159

mcepl · 2018-07-25T11:51:55Z

imp library has been deprecated since 3.4 and in 3.7 it finally breaks builds.

Preserving compatibility with python >= 2.7.

I have created a new test or updated the unit tests to cover the new/changed functionality.
No functionality has been changed or added.
I have updated master/src/CHANGES.txt directory (and read the README.txt in that directory)
I have updated the appropriate documentation

src/engine/SCons/Platform/__init__.py

bdbaddog · 2018-07-25T14:34:49Z

we've never supported python 3.4, so please remove support code for that.
It's Python 2.7.x & >= 3.5

bdbaddog · 2018-07-25T14:36:06Z

src/engine/SCons/Script/Main.py

-                # module's dictionary, so anything that code defines ends
-                # up adding to that module.  This is really short, but all
-                # the error checking makes it longer.
+                loader_details = (


Please add a brief comment as to what this does

Well, the same what the previous code ... opens module found in the particular location. See https://stackoverflow.com/a/37124336/164233

OK, I will mention it.

Also, this doesn't work.

[ 106s] + python3 bootstrap.py build/scons [ 106s] /usr/bin/python3 /home/abuild/rpmbuild/BUILD/scons-3.0.1/bootstrap/src/script/scons.py build/scons [ 107s] *** Error loading site_init file './site_scons/site_init.py': [ 107s] AttributeError: 'NoneType' object has no attribute 'loader': [ 107s] File "/home/abuild/rpmbuild/BUILD/scons-3.0.1/bootstrap/src/engine/SCons/Script/Main.py", line 1401: [ 107s] _exec_main(parser, values) [ 107s] File "/home/abuild/rpmbuild/BUILD/scons-3.0.1/bootstrap/src/engine/SCons/Script/Main.py", line 1364: [ 107s] _main(parser) [ 107s] File "/home/abuild/rpmbuild/BUILD/scons-3.0.1/bootstrap/src/engine/SCons/Script/Main.py", line 991: [ 107s] _load_all_site_scons_dirs(d.get_internal_path()) [ 107s] File "/home/abuild/rpmbuild/BUILD/scons-3.0.1/bootstrap/src/engine/SCons/Script/Main.py", line 844: [ 107s] _load_site_scons_dir(d) [ 107s] File "/home/abuild/rpmbuild/BUILD/scons-3.0.1/bootstrap/src/engine/SCons/Script/Main.py", line 782: [ 107s] mod = importlib.util.module_from_spec(spec) [ 107s] File "<frozen importlib._bootstrap>", line 580:

bdbaddog · 2018-07-25T14:36:55Z

src/engine/SCons/Tool/__init__.py

@@ -136,7 +137,7 @@ def _tool_module(self):
        sys.path = self.toolpath + sys.path
        # sys.stderr.write("Tool:%s\nPATH:%s\n"%(self.name,sys.path))

-        if sys.version_info[0] < 3 or (sys.version_info[0] == 3 and sys.version_info[1] in (0,1,2,3,4)):


Hmm. the py3 < 3.5 code shouldn't have ever been here..

I will gladly remove the other part of the condition, but that's what I've found in your code. Should I?

Yes. No code needed to support < 3.5 in the py 3 series.
This code is left over from when I did the bulk of the port before the decision was made to only support 3.5+

Well, this is primarily for Py2 code.

bdbaddog · 2018-07-25T14:39:32Z

We are getting almost 100% passing with py3.7 without these changes?
http://buildbot.scons.org/#/builders/13/builds/10/steps/1/logs/stdio

But if imp is obsolete, definitely worth moving forward!

mcepl · 2018-07-25T14:48:40Z

We had hundreds of build errors like this one.
_log.txt

mcepl · 2018-07-25T15:02:54Z

But if imp is obsolete, definitely worth moving forward!

https://docs.python.org/3/library/imp.html … “Deprecated since version 3.4: The imp package is pending deprecation in favor of importlib.”

bdbaddog · 2018-07-29T19:07:28Z

We are getting almost 100% passing with py3.7 without these changes?
http://buildbot.scons.org/#/builders/13/builds/10/steps/1/logs/stdio

But if imp is obsolete, definitely worth moving forward!

Looks like a bunch of test broke with py3.5 and 3.6. Can you take a look and resolve?

mcepl · 2018-07-30T08:23:07Z

Could we get this merged and then fix bugs which were already there?

bdbaddog · 2018-07-30T16:05:58Z

No. The policy is that tests should pass before merge.
Can you not fix it in your branch?
Commits there will update the Pull Request and initiate another regression run on travis.

It's currently breaking site_scons, which is pretty important.

mcepl · 2018-07-30T17:44:45Z

No. The policy is that tests should pass before merge.

Did tests pass without applying my patch? It seems to me that most of these failed tests are not my fault. Are they?

bdbaddog · 2018-07-30T18:30:15Z

Yes.
See the latest travis run from master:
https://travis-ci.org/SCons/scons/builds/408108763

bdbaddog · 2018-08-01T23:48:53Z

@mcepl Are you able to reproduce the test success without your pass with the various python versions?

mcepl · 2018-08-04T20:37:19Z

@mcepl Are you able to reproduce the test success without your pass with the various python versions?

I don't understand. Do you want me to play manually Travis/Appveyor? What's the point?

bdbaddog · 2018-08-04T21:20:38Z

SCons' regression tests pass on all versions of python (except pypy) without your change.
With your change the tests fail on PY3.5 and PY3.6

You asked if the tests passed without your changes and so I pointed you to travis-ci results which showed the failures from your changes.

If you look at this run: https://travis-ci.org/SCons/scons/builds/408108763
You'll see all the tests pass without your changes.

So please resolve the failures with your changes under py3.5 and 3.6 (and likely 3.7) or we cannot accept the pull request.

mcepl · 2018-08-06T16:27:49Z

SCons' regression tests pass on all versions of python (except pypy) without your change.

When running tests for master branch on my RHEL, I get these tests failing:

Failed the following 6 tests:
	test/packaging/option--package-type.py
	test/packaging/rpm/cleanup.py
	test/packaging/rpm/internationalization.py
	test/packaging/rpm/multipackage.py
	test/packaging/rpm/package.py
	test/packaging/rpm/tagging.py

See full log

bdbaddog · 2018-08-06T17:03:55Z

We don't have a RHEL build worker at this point. Regardless your changes cause new tests to fail on our regression machines. Please address. These are the newly failing tests with your changes I'm asking you to address: test/FindSourceFiles.py test/packaging/guess-package-name.py test/packaging/multiple-packages-at-once.py test/packaging/multiple-subdirs.py test/packaging/place-files-in-subdirectory.py test/packaging/sandbox-test/sandbox-test.py test/packaging/strip-install-dir.py test/packaging/tar/bz2_packaging.py test/packaging/tar/gz.py test/packaging/use-builddir.py test/packaging/zip.py test/site_scons/basic.py test/site_scons/site-dir.py test/site_scons/site_init.py Do these pass on your machine for py 2.7, 3.5, 3.6, 3.7 ? (They pass on our test machines for 2.7, but not for the other versions)

…

-Bill

On Mon, Aug 6, 2018 at 9:27 AM, Matěj Cepl ***@***.***> wrote: SCons' regression tests pass on all versions of python (except pypy) without your change. When running tests for master branch on my RHEL, I get these tests failing: Failed the following 6 tests: test/packaging/option--package-type.py test/packaging/rpm/cleanup.py test/packaging/rpm/internationalization.py test/packaging/rpm/multipackage.py test/packaging/rpm/package.py test/packaging/rpm/tagging.py See full log <https://github.com/SCons/scons/files/2263323/scons-runtest-log.txt> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3159 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFBNC_-dJK39cTNFD-JDwEgDVHqB7Tsks5uOG6FgaJpZM4Vf8SE> .

mwichmann · 2018-08-14T14:21:58Z

@mcepl: I have fixes for the rpm stuff in #3172, if you could try those it would be useful since we don't have a builder that will exercise that at present; hopefully taking those out of the equation would simplify sorting what's going on with the actual content of this patch.

src/engine/SCons/Tool/packaging/__init__.py

mwichmann · 2019-01-24T17:59:27Z

Adding comment from IRC which may shed some light on where problems occur

<rekado> Hi, I’m trying to package Scons for GNU Guix and I’m having some trouble with the bootstrap phase.
<rekado> I’m running python bootstrap.py build/scons with Python 3.7 and the first thing that happens is a complaint from Python about the use of “str” when a “byte-like” object is expected.
<rekado> The cause of this is rename_module, so I patched src/engine/SCons/compat/__init__.py to use __import__ instead of imp.load_module

mwichmann · 2019-03-05T15:49:04Z

Fwiw, this issue (use of "imp") completely blows up running the testsuite with Python 3.8 (still in alpha state), which has dialed up the intensity on warnings - on a large number of tests, the deprecation warning ends up in the stderr and so expected-output matches fail.

mwichmann · 2019-03-12T17:36:20Z

src/engine/SCons/Tool/packaging/__init__.py

@@ -122,8 +121,17 @@ def Package(env, target=None, source=None, **kw):
    # load the needed packagers.
    def load_packager(type):
        try:
-            file,path,desc=imp.find_module(type, __path__)
-            return imp.load_module(type, file, path, desc)
+            if PY2:


I think I have an alternate solution for this location... we just want a relative import as we're already in SCons/Tool/Packaging. For that, the following seems to work (for 2.7 as well)

try: return importlib.import_module("." + type, __name__)

src/engine/SCons/compat/__init__.py

src/engine/SCons/Platform/__init__.py

mcepl · 2019-03-13T07:28:40Z

Rebased on the top of the current master (387e1f3) and included ideas from the resolved reviews here.

mwichmann · 2019-03-13T14:07:44Z

My suggestions seem to have broken everything... odd because I thought I was running with the same things here and having it work.

mcepl · 2019-03-13T17:36:25Z

@mwichmann I really have problem here. I can do Python work pretty well (I am the lead Python maintainer for SUSE), but SCons seems to me to be so complicated I am really lost. If anybody who knows her way around SCons internals wants to take over this PR and lead it to some useable end, I would be more than happy.

mwichmann · 2019-03-13T18:04:46Z

I'll do some work on it. I can't really disagree with you, it's starting to feel comfortable for me in many parts now (and others remain complete mysteries), and that's after about two years of fiddling with it on and off. I don't want to offend Bill and the original developers, but it is too complex, sigh.

Several locations with simple usage of deprecated "imp" module changed to use "importlib". These match with work in SCons#3159, but this is not a complete implementation of SCons#3159. More regex patterns are changed to be raw strings. Some strings which did not seem appropriate to change to raw strings (e.g. contain embedded tabs, which Python should honor) had backslashes escaped to avoid accidental Python interpretation. Example: '\t<Import Project="$(VCTargetsPath)\\Microsoft.Cpp.targets" />\n' Python 3.8 was Warning \M was an unknown escape. More open().write(), open().read() style usage changed to use context managers so the file is closed. WIP part: even with Python 3.7, the tests which call sconsign.py fail; oddly they do not fail without the patch to compat.py. sconsign.py does an import using imp module (which is what generates the errors) so needs to be updated anyway. It does not quite fit the "simple usage" pattern - can't do a simple relative import since sconsign is normally located elsewhere in the tree than the main scons code body. With this version of the patch, 700 tests now pass with 3.8, and Warning messages reduced to 2800 (current master has 200 pass, 9000 warns) Signed-off-by: Mats Wichmann <mats@linux.com>

mwichmann · 2019-05-06T15:17:24Z

So as an update, some of the imp work has merged. I'm looking at the changes to Main.py now, and they're not passing the tests (python3 runtest.py test/site_scons) so will need some further thought. The error indicates that the code expects it will have found something, but does not, and so ends up accessing through a None object instead.

*** Error loading site_init file './site_scons/site_init.py':
AttributeError: 'NoneType' object has no attribute 'loader':

imp library has been deprecated since 3.4 and in 3.7 it finally breaks builds. Presrving compatibility with python >= 2.7.

bdbaddog · 2020-02-19T16:18:38Z

obsoleted by PR #3552

bdbaddog reviewed Jul 25, 2018

View reviewed changes

src/engine/SCons/Platform/__init__.py Outdated Show resolved Hide resolved

bdbaddog reviewed Jul 25, 2018

View reviewed changes

src/engine/SCons/Platform/__init__.py Outdated Show resolved Hide resolved

bdbaddog reviewed Jul 25, 2018

View reviewed changes

mwichmann reviewed Oct 5, 2018

View reviewed changes

src/engine/SCons/Tool/packaging/__init__.py Outdated Show resolved Hide resolved

mwichmann mentioned this pull request Mar 6, 2019

Support Python 3.8 #3299

Closed

mwichmann reviewed Mar 12, 2019

View reviewed changes

mwichmann mentioned this pull request Mar 14, 2019

[PY 3.8] fix more warnings #3331

Merged

3 tasks

mwichmann mentioned this pull request Apr 26, 2019

[wip] Py38warns4 tests #3345

Merged

3 tasks

mwichmann mentioned this pull request Jun 25, 2019

Update Script/Main.py so it uses importlib #3399

Closed

3 tasks

mcepl added 3 commits January 3, 2020 18:16

Replace use of imp library with importlib.

a10a1e7

imp library has been deprecated since 3.4 and in 3.7 it finally breaks builds. Presrving compatibility with python >= 2.7.

Include ideas from the review.

c8ab828

Remove side.review errors

05d7a3d

bdbaddog closed this Feb 19, 2020

mcepl deleted the replace-imp-with-importlib branch February 19, 2020 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace use of imp library with importlib. #3159

Replace use of imp library with importlib. #3159

mcepl commented Jul 25, 2018

bdbaddog commented Jul 25, 2018

bdbaddog Jul 25, 2018

mcepl Jul 25, 2018

mcepl Oct 5, 2018

bdbaddog Jul 25, 2018

mcepl Jul 25, 2018

bdbaddog Oct 5, 2018

mcepl Mar 13, 2019

bdbaddog commented Jul 25, 2018

mcepl commented Jul 25, 2018

mcepl commented Jul 25, 2018

bdbaddog commented Jul 29, 2018

mcepl commented Jul 30, 2018

bdbaddog commented Jul 30, 2018

mcepl commented Jul 30, 2018

bdbaddog commented Jul 30, 2018

bdbaddog commented Aug 1, 2018

mcepl commented Aug 4, 2018

bdbaddog commented Aug 4, 2018

mcepl commented Aug 6, 2018

bdbaddog commented Aug 6, 2018 via email

mwichmann commented Aug 14, 2018

mwichmann commented Jan 24, 2019

mwichmann commented Mar 5, 2019

mwichmann Mar 12, 2019

mcepl commented Mar 13, 2019

mwichmann commented Mar 13, 2019

mcepl commented Mar 13, 2019 •

edited

Loading

mwichmann commented Mar 13, 2019

mwichmann commented May 6, 2019 •

edited

Loading

bdbaddog commented Feb 19, 2020

Replace use of imp library with importlib. #3159

Replace use of imp library with importlib. #3159

Conversation

mcepl commented Jul 25, 2018

bdbaddog commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdbaddog commented Jul 25, 2018

mcepl commented Jul 25, 2018

mcepl commented Jul 25, 2018

bdbaddog commented Jul 29, 2018

mcepl commented Jul 30, 2018

bdbaddog commented Jul 30, 2018

mcepl commented Jul 30, 2018

bdbaddog commented Jul 30, 2018

bdbaddog commented Aug 1, 2018

mcepl commented Aug 4, 2018

bdbaddog commented Aug 4, 2018

mcepl commented Aug 6, 2018

bdbaddog commented Aug 6, 2018 via email

mwichmann commented Aug 14, 2018

mwichmann commented Jan 24, 2019

mwichmann commented Mar 5, 2019

Choose a reason for hiding this comment

mcepl commented Mar 13, 2019

mwichmann commented Mar 13, 2019

mcepl commented Mar 13, 2019 • edited Loading

mwichmann commented Mar 13, 2019

mwichmann commented May 6, 2019 • edited Loading

bdbaddog commented Feb 19, 2020

mcepl commented Mar 13, 2019 •

edited

Loading

mwichmann commented May 6, 2019 •

edited

Loading