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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

various build system fixes #1824

Merged
merged 7 commits into from Nov 15, 2018

Conversation

Projects
None yet
4 participants
@grondo
Copy link
Contributor

grondo commented Nov 9, 2018

Just parking this PR here for the moment in case someone else hits this issue.

If an existing libflux-core.so is installed in the system path (e.g. /usr/lib64), and flux-core is configured to side install to a different prefix (e.g. --prefix=/tmp/flux), make install will fail because, during relink, the linker will find the system libflux-core.so before the libflux-core.so that was just installed 馃憥.

e.g.:

.libs/py_mod.o: In function `register_pymod_service_name':
/tmp/flux-core/src/modules/pymod/py_mod.c:109: undefined reference to `flux_service_register'
collect2: error: ld returned 1 exit status
libtool: install: error: relink `pymod.la' with the above command before installing it
make[4]: *** [install-fluxmodLTLIBRARIES] Error 1

The problem is that there is a couple places where -l and -L flags are passed to target_LDFLAGS or AM_LDFLAGS, and I guess this is confusing libtool, which then promotes -L to the front of the arguments and if that happens to be -L/usr/lib64, the system path is searched before the prefix/lb path.

This patch just cleans up the few places I found where flags should have been put on _LDADD or _LIBADD.

@grondo grondo force-pushed the grondo:fix-ldflags branch from 264f9f8 to 5806199 Nov 9, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 9, 2018

Codecov Report

Merging #1824 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1824      +/-   ##
==========================================
- Coverage   79.84%   79.82%   -0.03%     
==========================================
  Files         197      197              
  Lines       35212    35212              
==========================================
- Hits        28116    28107       -9     
- Misses       7096     7105       +9
Impacted Files Coverage 螖
src/common/libutil/cronodate.c 95.1% <酶> (酶) 猬嗭笍
src/modules/barrier/barrier.c 76.55% <0%> (-2.07%) 猬囷笍
src/common/libflux/response.c 79.62% <0%> (-1.24%) 猬囷笍
src/common/libflux/mrpc.c 86.71% <0%> (-1.18%) 猬囷笍
src/broker/module.c 83.56% <0%> (-0.28%) 猬囷笍
src/common/libflux/message.c 81.15% <0%> (-0.25%) 猬囷笍
src/modules/kvs/kvs.c 65.57% <0%> (+0.15%) 猬嗭笍
@SteVwonder
Copy link
Member

SteVwonder left a comment

LGTM! Thanks @grondo!

@grondo grondo force-pushed the grondo:fix-ldflags branch 2 times, most recently from 90dea06 to 5a5ea5b Nov 14, 2018

@grondo grondo changed the title build: fix make install when an older libflux-core already installed on system various build system fixes Nov 14, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Nov 14, 2018

Ok, I've expanded this PR to include some other build system fixes, notably:

  • #1833: don't reference rcalc.o directly in LDADD
  • #1834: vpath builds fail because version.h can't be found in srcdir

Also, moved version.h to nodist_fluxcoreinclude_HEADERS so this generated file isn't distributed as part of make dist.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 14, 2018

LGTM (redefining G to reflect the unsatisfying available options :-)

Sorry this was such as PITA, and that I missed that version.h shouldn't be in the dist, which is pretty obvious now that you point it out.

grondo added some commits Nov 9, 2018

build: ensure -l, -L linker options are not in LDFLAGS
Problem: libtool appears to get befuddled when -L/path/to/foo -lfoo
type options appear in foobar_LDFLAGS, which is reserved for other
linker flags only. In these cases libtool might move a `-L` option
up in the argument vector, causing incorrect path to be first in the
linker path. Worst case, this can cause errors when trying to
side-install a newer version of flux-core when an old version exists
on the system. The relink will fail with unresolved symbols if a
-L/usr/lib or -L/usr/lib64 gets promoted up the argument vector.

While not a guaranteed fix, ensure that libraries and -L, -l link
options apper only in *_LIBADD or *_LDADD automake variables. This
seems to make it less likely that libtool will permute these
arguments, and works around the issue in most cases.
build: don't use wreck/rcalc.o directly in LDADD
Create librcalc.la convenience library Instead of including
wreck/rcalc.o directly in the LDADD of executables. This should
fix problems during parallel make of the form:

 gcc: error: rcalc.o: No such file or directory

Fixes #1833
libutil/cronodate: remove unused include of flux/core.h
Remove unnecessary include of flux/core.h header.
python: add --search option to make_binding.py
Problem: make_binding.py currently assumes all Flux API headers
are in the same directory, and this assumption doesn't hold for
generated headers like version.h, which may appear in a build
directory instead of source directory with other headers.

Add a new `--search` option to make_binding.py which can be used
to append to the script's search path, and internally build a list
of paths built from this path plus the "main" header path.
build: add build dir to include search paths
Problem: The flux core api header version.h is built by configure
and thus is not found under top_srcdir during a VPATH build.

Add

 -I$(top_builddir)/src/common/libflux

So that `#include "version.h"` finds libflux version.h during a
VPATH build, and in some cases

 -I$(top_builddir)

to handle `#include "src/common/libflux/version.h"`

For python bindings generation, ensure make_binding.py can find
the version.h header by adding

 `--search $(top_builddir)/src/common/libflux`

to the arguments for flux-core binding generation.

Fixes #1834
build/libflux: don't distribute generated version.h
version.h is generated by configure and so should not be added to
the distribution tarball. Move it from fluxcoreinclude_HEADERS
to nodist_fluxcoreinclude_HEADERS.
travis-ci: add --no-print-directory to make
Problem: The --output-sync flag to make is causing noisy output
from the build in travis, making it difficult to easily view tests
results.

Add the --no-print-directory flag if --output-sync is used to make
a little quieter.

Fixes #1837

@grondo grondo force-pushed the grondo:fix-ldflags branch from 57d9f9b to 2883258 Nov 14, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Nov 14, 2018

Rebased on current master.

garlick added a commit to garlick/flux-security that referenced this pull request Nov 14, 2018

build/lib: don't distribute generated version.h
Problem: version.h in included in make dist tarball,
but it is (re-)generated by configure.

Move it from fluxsecurityinclude_HEADERS to
nodist_fluxsecurityinclude_HEADERS.

This change tracks a similar change made in
flux-framework/flux-core#1824.

@garlick garlick merged commit 2bc53a7 into flux-framework:master Nov 15, 2018

2 of 3 checks passed

codecov/project 79.82% (-0.03%) compared to 4c5a8ae
Details
codecov/patch Coverage not affected when comparing 4c5a8ae...2883258
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@grondo grondo deleted the grondo:fix-ldflags branch Feb 8, 2019

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