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

Jump to bottom

Replace ipaplatform's symlinks with __path__ #169

Closed
wants to merge 1 commit into from
Closed

Replace ipaplatform's symlinks with __path__ #169

wants to merge 1 commit into from

Conversation

tiran
Copy link
Member

@tiran tiran commented Oct 18, 2016 •
edited by ghost

Signed-off-by: Christian Heimes cheimes@redhat.com

tiran
tiran commented Oct 18, 2016
View reviewed changes
ipaserver/dcerpc.py
@@ -58,7 +58,7 @@
import pysss_nss_idmap
import pysss
import six
from ipaplatform.paths import paths
from ipaplatform.paths import paths # pylint: disable=E0401
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reasons I do not understand, this import is not handled correctly by the pylint module extender.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use message ids (import-error) rather than message codes (E0401) in pylint comments.

tiran
tiran commented Oct 18, 2016
View reviewed changes
ipaplatform/__init__.py.in
@@ -5,8 +5,58 @@
'''
Module containing platform-specific functionality for every platform.
'''
import sys

NAME = "__PLATFORM__"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could get rid of text replacement and use Python's platform module instead. It reports fedora on Fedora and redhat on RHEL. I don't have a CentOS box to test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any benefit in deciding which platform module to use at runtime.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any benefits to hard-code the platform at build time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but that's the way we are doing it now and unless you have a good reason for the change, it will stay.

@pspacek
Copy link
Contributor

pspacek commented Oct 18, 2016

Uh, it is quite a lot of code to get rid of few symlinks. Is it worth? @jcholast @mbasti-rh @martbab @tomaskrizek and others?

@tiran
Copy link
Member Author

tiran commented Oct 18, 2016

I can also make the __init__.py.in go away and let everything be handled by auto-detecting the current distribution with Python's platform module.

HonzaCholasta
HonzaCholasta reviewed Oct 18, 2016
View reviewed changes
pylint_plugins.py
from ipaplatform.fedora import constants
from ipaplatform.fedora import paths
from ipaplatform.fedora import services
from ipaplatform.fedora import tasks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should import from ipaplatform.base here, as it defines the interface all platform modules should implement, including ipaplatform.fedora.

HonzaCholasta
HonzaCholasta reviewed Oct 18, 2016
View reviewed changes
ipaplatform/__init__.py.in
self.load_module(fullname)
return None

sys.meta_path.append(IpaPlatformImporter())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole change can be replaced with a single line:

__path__.append('__PLATFORM__')

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The __path__ trick results in a slightly different behavior:

__path__

>>> import ipaplatform.paths, ipaplatform.fedora.paths
>>> ipaplatform.paths.paths is ipaplatform.fedora.paths.paths
False

meta importer

>>> import ipaplatform.paths, ipaplatform.fedora.paths
>>> ipaplatform.paths.paths is ipaplatform.fedora.paths.paths
True

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior with __path__ is exactly the same as with the symlinks, which work just fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go for __path__ and hope that it won't break in the future. I'm going to ask Brett for his opinion on the trick later, too.

By the way your suggested alternative __path__.append('__PLATFORM__') does not work. Paths in __path__ must be relative to sys.path elements, not relative to the current package directory. The correct path is __path__.append(os.path.join(__name__, NAME)).

@tiran tiran force-pushed the ipaplatform_meta_importer branch 3 times, most recently from bfa3898 to f8012c0 Compare October 19, 2016 08:31
@pspacek
Copy link
Contributor

pspacek commented Oct 19, 2016

NACK

freeipa-server-4.4.90.201610190959GITf8012c0-0.fc24.x86_64 it broke following call which is used in RPM %post client scriptlet:

$ python2 -c 'from ipapython.certdb import update_ipa_nssdb; update_ipa_nssdb()'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib/python2.7/site-packages/ipapython/certdb.py", line 28, in <module>
    from ipaplatform.paths import paths
ImportError: No module named paths

@tiran
Replace ipaplatform's symlinks with a meta importer
d852c00 
Signed-off-by: Christian Heimes <cheimes@redhat.com>
@pspacek pspacek self-assigned this Oct 19, 2016
@tiran tiran changed the title Replace ipaplatform's symlinks with a meta importer Replace ipaplatform's symlinks with __path__ Oct 19, 2016
@pspacek pspacek added the ack Pull Request approved, can be merged label Oct 20, 2016
@ghost
Copy link

ghost commented Oct 20, 2016

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/8f98fa1bd5f1da207fab6f89b75e0cdc19d00797

@ghost ghost added the pushed Pull Request has already been pushed label Oct 20, 2016
@ghost ghost closed this Oct 20, 2016
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HonzaCholasta HonzaCholasta HonzaCholasta left review comments

Assignees

@pspacek pspacek

Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
3 participants
@tiran @pspacek @HonzaCholasta