libMesh / libmesh

libMesh github repository
http://libmesh.github.io
GNU Lesser General Public License v2.1
654 stars 286 forks source link

Removal of irrelevant pkgconfig files. #2679

Open pshriwise opened 4 years ago

pshriwise commented 4 years ago

I remember discussing this earlier in the year with some devs, but sadly that conversation was lost due to slack message history limitations. This is my account of that converstaion as best I can recall it:

Currently, it appears that all of the libmesh-*.pc files get installed with libmesh even if some of the library builds are disabled. This makes it impossible for an external application to determine which of these libraries are actually present in the install until it potentially fails to find them during linking. In particular, it would be nice to external apps to be able to select which libMesh library to link to based on its own configuration.

It seems that an appropriate fix would be to only install the libmesh-*.pc files that are relevant to the build configuration with an always-present libmesh.pc that refers to an available configuration in the installation with a preference of: opt, devel, dbg, oprof, prof. (Not sure that's an ideal order for the last two configuration options honestly, just my best guess.)

jwpeterson commented 4 years ago

Definitely sounds like something it would be good to fix. Within our autoconf files, once the LIBMESH_SET_METHODS (m4/libmesh_methods.m4) function has been called, the Makefile variables LIBMESH_OPT_MODE, LIBMESH_DBG_MODE, LIBMESH_DEVEL_MODE, LIBMESH_PROF_MODE, LIBMESH_OPROF_MODE are set.

The relevant lines in Makefile.am are then:

# Support for pkgconfig
pkgconfigdir     = $(libdir)/pkgconfig
pkgconfig_DATA   = contrib/utils/Make.common \
                   contrib/utils/libmesh.pc \
                   contrib/utils/libmesh-opt.pc \
                   contrib/utils/libmesh-dbg.pc \
                   contrib/utils/libmesh-devel.pc \
                   contrib/utils/libmesh-prof.pc \
                   contrib/utils/libmesh-oprof.pc

configdir        = $(sysconfdir)/libmesh
config_DATA      = contrib/utils/Make.common \
                   contrib/utils/libmesh.pc \
                   contrib/utils/libmesh-opt.pc \
                   contrib/utils/libmesh-dbg.pc \
                   contrib/utils/libmesh-devel.pc \
                   contrib/utils/libmesh-prof.pc \
                   contrib/utils/libmesh-oprof.pc

So one thing you could try is wrapping these Makefile.am lines in conditional statements, e.g.

if LIBMESH_OPT_MODE
pkgconfig_data += contrib/utils/libmesh-opt.pc
config_DATA += contrib/utils/libmesh-opt.pc
endif

and so on for the other METHODs. This should make it so only the pc files for the relevant METHODs are actually installed by make install.

pshriwise commented 4 years ago

Simple enough. I'd be happy to take a crack at it.

pshriwise commented 4 years ago

Alright, so adding those conditionals was no problem. Now for the libmesh.pc file...

It would be nice to have those LIBMESH_*_MODE variables around when setting the list of configuration files in configure.ac. @jwpeterson, do you see any issues with moving the LIBMESH_SET_METHODS call earlier in that file?

https://github.com/libMesh/libmesh/blob/a088573020e6301c6fc2a3e272e13a961a611668/configure.ac#L32-L48

jwpeterson commented 4 years ago

Hmm... my gut feeling is that it's never really safe to rearrange the order of m4 function calls. This is one of the major shortcomings of autoconf... basically all functions have access to the same global namespace of shell variables, so changing the order of function calls can have far-reaching effects.

Would it not be OK to still keep the full list of pc files in AC_CONFIG_FILES as long as we only install the ones that are actually used? Or is there some issue with that approach?

pshriwise commented 4 years ago

The issue with that is the libmesh.pc file. It currently always matches the libmesh-opt.pc file, but, as we've established, those libraries may not always be present. So the libmesh.pc should be configured using the libmesh-opt/dbg/devel/prof/oprof.pc/in file as appropriate.

You're probably right about not moving that LIBMESH_SET_METHODS call. Maybe an additional call to AM_CONFIG_FILES after LIBMESH_SET_METHODS would be better. I'm thinking that adding a LIBMESH_PC_IN variable in LIBMESH_SET_METHODS would be the move here. Then a call like

AC_CONFIG_FILES([contrib/utils/libmesh.pc:$LIBMESH_PC_IN])

to set that correctly.

jwpeterson commented 4 years ago

I see, I guess I didn't really consider the case where you wouldn't install at least an opt build, but I guess it is possible so we should make sure that it works correctly.

Multiple calls to AC_CONFIG_FILES is definitely fine since we apparently already do that, so your idea seems reasonable to me, provided there's no weirdness with using a shell variable in AC_CONFIG_FILES... I assume that should work fine, though.