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

build: do not install libflux-rdl.so, fix rebuild of aclocal.m4 on first make #421

Merged
merged 4 commits into from Jan 8, 2019

Conversation

Projects
None yet
3 participants
@grondo
Copy link
Contributor

grondo commented Jan 4, 2019

This PR turns libflux-rdl.so into a convenience library instead of an installed library as discussed in #420. It was simpler to keep the other requisite convenience libs (libutil, liblsd) as part of libflux-rdl.la and remove them when duplicated in LIBADD, than to leave them out of libflux-rdl.la and then add them by hand everywhere libflux-rdl.la is used. However, I do have a working implementation of the other way if that is preferred.

I also got annoyed and fixed the issue where make thought it had to rebuild $(srcdir)/aclocal.m4 after every autogen.sh && ./configure (this was because autogen.sh was moving it to where it couldn't be found)

rdl: do not install libflux-rdl.so
Problem: There are no external users of libflux-rdl.so, but the
library is distributed.

Make libflux-rdl an internal convenience library instead of an
installed one. Keep the convenience libraries liblsd.la and libutil.la
inside of libflux-rdl.la and remove these on LIBADD lines where
necessary to avoid duplicate symbol errors.

Fixes #420
@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jan 4, 2019

This could be because the AUX_DIR and CONFIG_DIR are different in flux-sched (m4 and config
respectively).

Odd... Do you know why sched ends up using both AUX_DIR and CONFIG_DIR? It seems keeping all m4 scripts in config might make thing more readable and consistent with flux-core.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 4, 2019

Codecov Report

Merging #421 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #421   +/-   ##
=======================================
  Coverage   75.62%   75.62%           
=======================================
  Files          67       67           
  Lines       10992    10992           
=======================================
  Hits         8313     8313           
  Misses       2679     2679

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a89b170...f6bc89d. Read the comment docs.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jan 4, 2019

@grondo: LGTM! Thanks.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jan 4, 2019

Odd... Do you know why sched ends up using both AUX_DIR and CONFIG_DIR? It seems keeping all m4 scripts in config might make thing more readable and consistent with flux-core.

Yeah, I'm not entirely sure, I think it has diverged from the beginning though. Should we just fix that in this PR as well?

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jan 4, 2019

If that is not that time consuming, I think it makes sense just fixing it as part of this PR. Thanks.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jan 7, 2019

Ok, I've pushed a commit that moves the contents of m4 into config to more closely match existing practice in flux-core

@grondo grondo force-pushed the grondo:issue#420 branch from 3072760 to e8e8678 Jan 7, 2019

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jan 7, 2019

@grondo: Great! I will take a look at the PR after my meeting at 10AM and merge if no other changes are required. Thanks.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jan 7, 2019

@grondo: m4 --> config looks great to me. One thing though, with this move, do we want to revert the 084b433 change so that flux-sched's augogen.sh becomes more consistent with flux-core?

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jan 8, 2019

m4 --> config looks great to me. One thing though, with this move, do we want to revert the 084b433 change so that flux-sched's augogen.sh becomes more consistent with flux-core?

Ah, yeah, good point. I will rework those changes so we can use the same autogen.sh

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jan 8, 2019

Thanks.

grondo added some commits Jan 4, 2019

rdl: remove rdl_version.map
libflux-rdl is no longer a shared library, so rdl_version.map is not
used. Remove it.
build: combine m4/ and config/ dirs
Move the autotools macros and other ancillary files from the m4/
directory into the config/ directory to more closely mimic the setup
in flux-core. Using a separate AC_MACRO directory caused confusion
without much benefit.

Copy a simple autogen.sh from flux-core to reduce differences between
the projects.
configure: add AM_MAINTAINER_MODE([enable])
Add

 AM_MAINTAINER_MODE([enable])

as suggested at

 https://autotools.io/automake/maintainer.html

This adds --disable-maintainer-mode option to configure, without
disabling maintainer-mode by default (which is discouraged). This
also matches what is done in flux-core.

@grondo grondo force-pushed the grondo:issue#420 branch from e8e8678 to f6bc89d Jan 8, 2019

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jan 8, 2019

Ok, I've forced a push of this branch with 084b433 removed.

I've also added a commit that adds AM_MAINTAINER_MODE([enable]) to configure.ac, which will enable a --disable-maintiner-mode option to ./configure which may be required for some packaging systems. This also matches what flux-core will have (once I submit a small PR there)

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jan 8, 2019

Thanks @grondo. Yes, AM_MAINTAINER_MODE([enable]) is really a good addition. I vaguely remember I had to add this to avoid some timestamp mishap.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jan 8, 2019

@grondo: Hmmm, for some reason I'm still seeing your autogen.sh change in 860b063...

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jan 8, 2019

@grondo: Hmmm, for some reason I'm still seeing your autogen.sh change in 860b063...

You mean that commit is still in this branch? I did force push, so you might have an old copy.

If you mean that the move of aclocal.m4 is still removed from autogen.sh, then that is intentional because it was the root cause of the problem where make would rerun configure the first time after autogen.sh (aclocal.m4 needs to be at the top level)

This will make autogen.sh from flux-core and flux-sched match after flux-framework/flux-core#1912 is merged.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jan 8, 2019

If you mean that the move of aclocal.m4 is still removed from autogen.sh, then that is intentional because it was the root cause of the problem where make would rerun configure the first time after autogen.sh (aclocal.m4 needs to be at the top level)

Ah I see. I somehow mistakenly thought that aclocal.m4 being moved to config directory is okay when you don't have both AUX_DIR and CONFIG_DIR, not leading to a redundant configure rerun . I will merge as is then.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jan 8, 2019

I somehow mistakenly thought that aclocal.m4 being moved to config directory is okay when you don't have both AUX_DIR and CONFIG_DIR, not leading to a redundant configure rerun

I thought that at first as well. However, if you look at aclocal.m4 location in generated Makefiles, you find it should be in top_srcdir:

grondo@peep:~/git/flux-sched$ grep aclocal\\.m4 Makefile
ACLOCAL_M4 = $(top_srcdir)/aclocal.m4

I'm not sure why -I config doesn't change that, but I didn't want to fight it too much. It was easier to just leave aclocal.m4 where ./autoreconf thought it should go...

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jan 8, 2019

Agreed.

@dongahn dongahn merged commit 26fe74a into flux-framework:master Jan 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@grondo grondo deleted the grondo:issue#420 branch Jan 11, 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.