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

Workaround for py2 builtin, =<3.3 imp and >=3.4 libimport quirks #50128

Merged
merged 10 commits into from Nov 5, 2018

Conversation

Projects
None yet
3 participants
@mgomersbach
Copy link
Contributor

commented Oct 19, 2018

What does this PR do?

Python 2 has a builtin for "reload"
Python lower than 3.3 has the "imp" module
And Python 3.4 and up switched to "importlib"
This patches to fall back through the different implementations

What issues does this PR fix or reference?

solves #50127

Previous Behavior

NameError: name 'reload' is not defined

New Behavior

Works

Tests written?

No

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@gtmanfred
Copy link
Contributor

left a comment

Can these not be used as imports at the top of the file?

Check if reload is in globals(), if it is not, then import it with the ImportError exception?

@mgomersbach

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2018

Yeah my reasoning was to not evaluate it every time the module was called.
I'll change the PR!

mgomersbach added some commits Oct 19, 2018

@mgomersbach

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

The test result seems a fluke? Can it be retriggered?

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

Those failures are unrelated.

@gtmanfred
Copy link
Contributor

left a comment

Can we do something more like.

if not hasattr(globals()['__builtins__'], 'reload'):
    try:
        from importlib import reload
    except ImportError:
        from imp import reload

Also, have you verified that the builtins work on both python2 and py3? i know the handling of builtins was changed in python3

@mgomersbach

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2018

Yes 2.7, <=3.3 and >=3.4 have been tested for that code. (does salt run on <=3.3?, because we can then simplify it even more)

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2018

ok, perfect, then i would still ask that it be changed to be in an if statement instead of doing to try blocks.

@mgomersbach

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2018

Will do when I arrive at my destination!
Final question, does saltstack run on python 3.3 anywhere because I can leave out the except then

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2018

We do not run on 3.3, we support 2.7 and >= 3.4

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2018

But I do believe we use imp instead of importlib on python3.4 for our loader, so that would probably be good to use there.

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2018

Also, probably better to just check the python version instead of using try except blocks.

@mgomersbach

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

imp is deprecated since 3.4.
Version checking would exclude any other versioning from alternative pythons (unless I make a big if else tree).
When I have access to a desktop I will implement your suggestions

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

@mgomersbach Since we only support python == 2.7 and python >= 3.4, it shouldn't be too big of a tree. You can use the six.PY2 and six.PY3 variables, available from salt.ext.six.

mgomersbach added some commits Nov 1, 2018

@mgomersbach

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

E8305 in ebuild.py:250 is easy to fix
But what to do about on E0611 in ebuild.py:32 and virtualbox.py:22 ?

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2018

if you want to just disable that check for those two lines, that should be good enough, since you are explicitly only running the import on versions where that import is there.

pylint runs on python2, so it doesn't know that it is wrong.

mgomersbach added some commits Nov 2, 2018

@mgomersbach

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2018

Since the checks are ok now, do you want me to rebase, squash and reword the commits?

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2018

@mgomersbach You can if you'd like to, but you don't have to. Let me know what you want to do.

@mgomersbach

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

Then let's not. Comments provide enough context I guess.

@rallytime rallytime merged commit cd7c95f into saltstack:2017.7 Nov 5, 2018

6 of 10 checks passed

jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has failed
Details
continuous-integration/jenkins/pr-merge This commit is being built
Details
jenkins/pr/py2-ubuntu-1604 running py2-ubuntu-1604...
Details
WIP Ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details

@mgomersbach mgomersbach deleted the mgomersbach:fix-py3-reload-compat branch Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.