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

configure: remove --disable-python #1772

Merged
merged 3 commits into from Oct 27, 2018

Conversation

Projects
None yet
4 participants
@grondo
Copy link
Contributor

grondo commented Oct 26, 2018

Python bindings are a first-class tool in flux-core now, and checking for HAVE_PYTHON everywhere is getting cumbersome. This PR removes the --disable-python configure flag and makes python a hard requirement.

One question, I removed the bit of code that set an rpath in the broker for the location of libpython.

diff --git a/src/broker/Makefile.am b/src/broker/Makefile.am
index a8543c37..548a4d53 100644
--- a/src/broker/Makefile.am
+++ b/src/broker/Makefile.am
@@ -57,14 +57,7 @@ flux_broker_LDADD = \
        $(top_builddir)/src/common/libpmi/libpmi.la \
        $(LIBPMIX)
 
-broker_ldflags = 
-
-if HAVE_PYTHON
-#allow the broker to find the python library at runtime
-broker_ldflags += -rpath $(PYTHON_PREFIX)/lib
-endif
-
-flux_broker_LDFLAGS = ${broker_ldflags}
+flux_broker_LDFLAGS =
 
 TESTS = test_shutdown.t \
        test_heartbeat.t \

The broker doesn't link against libpython, so I didn't see how it actually accomplished anything.

Maybe this rpath needs to be set on pymod? Anyway, I didn't hit any trouble removing it, but perhaps it is something that only crops up in environments with multiple pythons..

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 26, 2018

Oh, I also removed travis-dep-builder.sh in this PR (didn't want to make a separate PR for it). It is now out-of-date and unused.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 26, 2018

Oops, I may have typoed something in one of the Makefiles. Let me check on it...

@grondo grondo force-pushed the grondo:disable-disable-python branch from 27aabaf to 418c574 Oct 26, 2018

@grondo grondo requested a review from trws Oct 26, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 26, 2018

Ok, fixed that typo. It would be good if @trws reviewed this PR specifically the change to remove -rpath on the broker.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 26, 2018

Codecov Report

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

@@            Coverage Diff             @@
##           master    #1772      +/-   ##
==========================================
+ Coverage   79.54%   79.55%   +0.01%     
==========================================
  Files         185      185              
  Lines       34477    34477              
==========================================
+ Hits        27425    27429       +4     
+ Misses       7052     7048       -4
Impacted Files Coverage Δ
src/common/libflux/message.c 81.26% <0%> (+0.12%) ⬆️
src/modules/barrier/barrier.c 78.62% <0%> (+2.06%) ⬆️
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 26, 2018

@trws if this looks good to you, please push the button.

@trws

This comment has been minimized.

Copy link
Member

trws commented Oct 26, 2018

I'm good with all of this, but admittedly a bit nervous about removing the rpath. Perhaps have it, but only have it if the user explicitly specifies a path for python or something, or just on pymod? The worry is that it's been pretty common to use a python other than the primary python on the system, it was the standard for us on toss2 and I know at least some users are still doing it. Maybe I'm being over cautious, but having flux load the wrong python at runtime could cause some odd problems.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 26, 2018

The worry is that it's been pretty common to use a python other than the primary python on the system, it was the standard for us on toss2 and I know at least some users are still doing it. Maybe I'm being over cautious, but having flux load the wrong python at runtime could cause some odd problems.

I guess I was wondering why the broker was linked with the -rpath. I could be missing something fundamental, but the broker doesn't load or link with any python libraries or python code. Right now pymod is the only thing I can think of that actually links against libpython (besides the bindings themselves). So should the rpath actually be added to pymod.so?

@trws

This comment has been minimized.

Copy link
Member

trws commented Oct 26, 2018

Yes, that would make more sense. I can't remember for sure why I did it the other way around, maybe originally hacking to make pymod's functionality part of the core module loader? Either way, yes.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 26, 2018

Ok, I will move the rpath to pymod.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 26, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 26, 2018

Ugh. That broke make install

Ok, I can't believe I didn't know this until now, but libtool has its own -rpath option which is not exactly the same as the linker's -rpath option (it behaves differently between a program and a library). libtool gets confused if you supply -rpath on a module, so I'm going to try -Wl,-rpath -Wl PATH instead.

@grondo grondo force-pushed the grondo:disable-disable-python branch from 6c72ae3 to a94ad8b Oct 26, 2018

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 26, 2018

One builder failed with a write error on stdout, so restarting

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 26, 2018

Just needs a rebase and then IMHO should be merged.

@trws

trws approved these changes Oct 26, 2018

Copy link
Member

trws left a comment

:shipit: (edit: it occurs to me I haven't used this here before, in case that was opaque, that's :shipit:, or LGTM or what have you, why shipit is a squirrel I do not remember...)

grondo added some commits Oct 26, 2018

configure: remove --disable-python
Python is now supported as a first-class binding in flux-core, and more
utilities and tests will require its presence. Acknowledge that fact
and remove the --disable-python flag from ./configure. Python and python-devel,
etc are now requirements for the build.

Fixes #1767
src/test/travis-dep-builder.sh: remove
This script is unused and out of date. Remove it to avoid confusion.
pymod: add rpath for python library to pymod.so
Problem: There is a chance that pymod will link with the wrong
libpython at runtime on a system with many python versions.

Ensure pymod links with the same libpython found by configure by
embedding an rpath to the library at link time.

@grondo grondo force-pushed the grondo:disable-disable-python branch from a94ad8b to 92df0a6 Oct 27, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 27, 2018

rebased

@garlick garlick merged commit 426227e into flux-framework:master Oct 27, 2018

3 checks passed

codecov/patch Coverage not affected when comparing 8bc27cf...92df0a6
Details
codecov/project 79.55% (+0.01%) compared to 8bc27cf
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@grondo grondo deleted the grondo:disable-disable-python branch Feb 8, 2019

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.