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

ensure system python path is not pushed to front of PYTHONPATH #2144

Merged
merged 3 commits into from May 2, 2019

Conversation

@grondo
Copy link
Contributor

commented May 1, 2019

First attempt at solving #1999.

Ensure that a system python path is never pushed to the front of PYTHONPATH by installing links to Flux directories installed under $pyexecdir in a non-standard path. The flux(1) cmddriver can then prepend this path to PYTHONPATH instead of a possible system path which could result in overriding user preference. As discussed in #1999, this approach still allows import flux using the system python since the modules are still really installed in the default pyexecdir.

This seems to work as intended and doesn't break any tests (I think). However, definitely one of our python gurus should check it out.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

This branch breaks make distcheck. I'll look into it.

@grondo grondo force-pushed the grondo:issue#1999 branch 2 times, most recently from 8c06907 to 7800eee May 1, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Ok, fixed the issue with make distcheck (wish I knew the "right" way to install a symlink with automake, the current approach does it by hand)

@@ -356,6 +358,13 @@ AC_SUBST(fluxcmddir)
AS_VAR_SET(fluxlibdir, $libdir/flux)
AC_SUBST(fluxlibdir)

# Target of PYTHONPATH set by flux(1) cmddriver, so flux(1)
# doesn't inadvertently insert system python paths (or any
# other pytho path for that matter) first in PYTHONPATH.

This comment has been minimized.

Copy link
@chu11

chu11 May 1, 2019

Contributor

typo pytho

This comment has been minimized.

Copy link
@grondo

grondo May 1, 2019

Author Contributor

Thanks!

@@ -251,10 +251,12 @@ if test "$ac_python_ldflags_set_by_user" != "true"; then
PYTHON_LDFLAGS="-l$ac_python_library"
fi
fi
AS_VAR_SET(fluxpyexecdir, $pyexecdir/flux)
AC_SUBST(fluxpydir)

This comment has been minimized.

Copy link
@chu11

chu11 May 1, 2019

Contributor

should this be fluxpyexecdir?

This comment has been minimized.

Copy link
@grondo

grondo May 1, 2019

Author Contributor

Good catch! Actually that is a rebase error and those lines need to be removed (fluxpyexecdir was from an earlier version of the PR)

@grondo grondo force-pushed the grondo:issue#1999 branch from 7800eee to fb8f6ec May 1, 2019
Change the name of the flux python dso install directory from
'fluxsodir' to 'fluxpysodir' so it is clear that it is a variable
specific to python binding installation.
@grondo grondo force-pushed the grondo:issue#1999 branch from fb8f6ec to da9d673 May 1, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Thanks @chu11! Rebased addressing your comments.

@chu11

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

LGTM, but I think a python expert should probably take a glance too (@trws or @SteVwonder)

@SteVwonder

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Thanks @grondo for putting this together!

Gave this a spin, and it works well. My only concern is what happens when we configure flux for multiple pythons.

Right now, this is what happens when you configure/install for python3.7 and then again for python2.7:

# tree $PREFIX/lib

lib
├── flux
│   └── python
│       ├── flux -> $PREFIX/lib/python2.7/site-packages/flux
│       └── _flux -> $PREFIX/lib/python2.7/site-packages/_flux
└── python2.7
    └── site-packages
        ├── flux
        ├── _flux
        ├── <every other python2.7 system-installed package>
└── python3.7
    └── site-packages
        ├── flux
        ├── _flux
        ├── <every other python3.7 system-installed package>

So flux python will work, launching the python that flux was configured with and not pollute the PYTHONPATH with a system path, but if a user wants to use a python3.7 interpreter and import flux, they cannot do so easily without including a "polluted" system path in their PYTHONPATH (i.e., /usr/lib/flux/python3.7/site-packages).

Can we tweak fluxpylinkdir to have the major/minor version numbers at the end of the directory name so that this workaround works when there are multiple python versions?

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

but if a user wants to use a python3.7 interpreter and import flux, they cannot do so easily without including a "polluted" system path in their PYTHONPATH (i.e., /usr/lib/flux/python3.7/site-packages).

Isn't this the same as any other python package installed on the system? How do users normally import a system installed package without "polluting" their PYTHONPATH?

I don't think I had the use case in mind that users would want to use /usr/lib/flux/python as a way to "isolate" the flux python bindings by manually adding that path to PYTHONPATH. This solution is only to isolate the flux PYTHONPATH for flux itself.

Since flux(1) can only be compiled to support a single version of python, I thought it appropriate to just have a single directory.

That being said, there probably isn't any harm in adding the python version to /usr/lib/flux/python, assuming the python version is easily available in configure.ac this should be trivial.

@SteVwonder

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Isn't this the same as any other python package installed on the system? How do users normally import a system installed package without "polluting" their PYTHONPATH?

Yes, it is how other system-installed python packages are added. Normally, if a user wants to import a specific package without pulling in system packages, they will install it locally with pip install --user PACKAGE_NAME (or install it with pip into a virtualenv). Currently, we don't put our python bindings up on PyPI (the Python Package Index), so that's not an option for our users. Adding our bindings to PyPI is probably the better long-term solution (created #2145 to track that), but I'm wonder if this would be sufficient for the short-term.

This solution is only to isolate the flux PYTHONPATH for flux itself.

Yeah, this PR solves #1999 perfectly. So if we want to leave how it is, that is fine.

assuming the python version is easily available in configure.ac this should be trivial.

It "should be" ™️ as simple as referencing PYTHON_VERSION [1]

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Yes, as you hinted, the following patch (pushed as a separate commit for now) just seems to work:

diff --git a/configure.ac b/configure.ac
index 57d45b781..224ff43c3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -360,7 +360,7 @@ AC_SUBST(fluxlibdir)
 # doesn't inadvertently insert system python paths (or any
 # other python path for that matter) first in PYTHONPATH.
 #
-AS_VAR_SET(fluxpylinkdir, $fluxlibdir/python)
+AS_VAR_SET(fluxpylinkdir, $fluxlibdir/python$PYTHON_VERSION)
 AC_SUBST(fluxpylinkdir)
 
 AS_VAR_SET(fluxmoddir, $libdir/flux/modules)

Let me know what you think and I can squash that one down.

Copy link
Member

left a comment

Thanks @grondo! Works like a charm.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Great I'll squash that last commit

grondo added 2 commits Apr 30, 2019
Add a links to the Flux python modules installed under the standard
python module path, pyexecdir, in a non-standard path. This will allow
the flux(1) cmddriver to prepend a path to PYTHONPATH that *only*
has the Flux python modules in it, instead of potentially pulling
in other modules unnecessarily.
Change the INSTALLED_PYTHON_PATH conf variable to use
`fluxpylinkdir` instead of `pexecdir`. This will allow the
flux(1) cmddriver to push a non-standard path to the front of
PYTHONPATH, pulling in just the flux python modules without
inadvertently pulling in other unwanted modules.

Fixes #1999
@grondo grondo force-pushed the grondo:issue#1999 branch from 362a856 to 749df2f May 1, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

squashed.

@codecov-io

This comment has been minimized.

Copy link

commented May 1, 2019

Codecov Report

Merging #2144 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2144      +/-   ##
==========================================
+ Coverage   80.41%   80.45%   +0.03%     
==========================================
  Files         200      200              
  Lines       31781    31781              
==========================================
+ Hits        25557    25569      +12     
+ Misses       6224     6212      -12
Impacted Files Coverage Δ
src/cmd/flux-module.c 83.96% <0%> (+0.23%) ⬆️
src/modules/job-ingest/worker.c 72.72% <0%> (+7.69%) ⬆️
@SteVwonder

This comment has been minimized.

Copy link
Member

commented May 2, 2019

LGTM! Ready for merging?

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

LGTM! Ready for merging?

Yes, thanks!

@garlick garlick merged commit bf2b9d7 into flux-framework:master May 2, 2019
2 checks passed
2 checks passed
Mergify — Summary 1 potential rule
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo grondo deleted the grondo:issue#1999 branch May 2, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Thanks!

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