Cleanup a number of things with plugins:

[opoplawski]

• Drop unneeded rpath
• Drop unneeded so version-info
• Handle test plugins by removing them after install

Working on ndfcdf 4.9.0 for Fedora presented a number of issues - the first of which was installing a bunch of plugins into the DESTDIR rpm BUILDROOT in locations like /home/orion/.... After trying a few different things I settled on the scheme of removing the test plugins after install as the simplest.

Next, plugins shouldn't have SO version info so I removed that.

Finally, rpath is generally banned in Fedora so I removed that, and I'm pretty sure it wouldn't be needed elsewhere but perhaps I am mistaken.

[DennisHeimbigner]

Unfortunately, we do not routinely test on Fedora. Perhaps we should. In any case, I have a couple of comments.

1. The rpath was mostly a result of copying some code I found a long time ago when first building plugins. I would worry about what happens with MINGW (and Cygwin), but since the tests passed, I assume it is ok.
2. If you remove the so version (0.0.0), then what does the library name look like: "lib__nch5deflate.so" or what? Since I assume you ran the tests successfully, my worry that it might affect some tests is probably unfounded.
3. This code:

CHECKLIBS = lib__nch5noop.la lib__nch5noop1.la ... plugin_LTLIBRARIES += ${CHECKLIBS}

This appears to cause testing libraries (like noop1) to get installed when they should not be installed, Do I have that right?

[opoplawski]

2. If you remove the so version (0.0.0), then what does the library name look like: "lib__nch5deflate.so" or what? Since I assume you ran the tests successfully, my worry that it might affect some tests is probably unfounded.

Yes, the library is simply lib__nch5deflate.so. Since that is presumably the name that is used to load it, that seems appropriate. Having lib__nch5deflate.so.0.0.0 is superfluous.

3. This code:

CHECKLIBS = lib__nch5noop.la lib__nch5noop1.la ... plugin_LTLIBRARIES += ${CHECKLIBS}

This appears to cause testing libraries (like noop1) to get installed when they should not be installed, Do I have that right?

They do get copied to the install location by install-data, but then they are removed immediately by installl-data-hook. A comment about that is probably appropriate...

[opoplawski]

Another issue - it looks like lib__nczhdf5filters.so is not linked to libnetcdf:

netcdf.x86_64: W: undefined-non-weak-symbol /usr/lib64/hdf5/plugin/lib__nczhdf5filters.so nc_inq_var_chunking   (/usr/lib64/hdf5/plugin/lib__nczhdf5filters.so)
netcdf.x86_64: W: undefined-non-weak-symbol /usr/lib64/hdf5/plugin/lib__nczhdf5filters.so nc_inq_dimlen (/usr/lib64/hdf5/plugin/lib__nczhdf5filters.so)
netcdf.x86_64: W: undefined-non-weak-symbol /usr/lib64/hdf5/plugin/lib__nczhdf5filters.so nc_inq_type   (/usr/lib64/hdf5/plugin/lib__nczhdf5filters.so)
netcdf.x86_64: W: undefined-non-weak-symbol /usr/lib64/hdf5/plugin/lib__nczhdf5filters.so nc_inq_var_endian     (/usr/lib64/hdf5/plugin/lib__nczhdf5filters.so)
netcdf.x86_64: W: undefined-non-weak-symbol /usr/lib64/hdf5/plugin/lib__nczhdf5filters.so nc_inq_var    (/usr/lib64/hdf5/plugin/lib__nczhdf5filters.so)

I've added a commit for that.

[opoplawski]

There is also the question of how the package netcdf's HDF5 plugins. Presumably aside from nczhdf5filters the plugins are useful without netcdf installed - is that correct?

[DennisHeimbigner]

There is also the question of how the package netcdf's HDF5 plugins. Presumably aside from nczhdf5filters the plugins are useful without netcdf installed - is that correct?

True if someone wanted to use them with HDF5, but not using netcdf-c. But I am not sure that is within Unidata's remit.

[DennisHeimbigner]

As for installing our test filters and then deleting them, I would prefer to not install them at all using the current mechanism.

[opoplawski]

The current system is a pain for packagers because all of the filters get installed into DESTDIR and need to get manually removed. This method deletes them automatically.

[DennisHeimbigner]

The current system is a pain for packagers because all of the filters get installed into DESTDIR and need to get manually removed. This method deletes them automatically.

I am not sure I understand. You can suppress the installation of the filters using --without-plugin-dir. Also, in the current 4.9.1, it should be the case that the testing filters are not being installed at all. Are you seeing differently?

[DWesl]

Would putting the test plugins in check_LTLIBRARIES instead of plugin_LTLIBRARIES, lib_LTLIBRARIES, or tmp_LTLIBRARIES work, or does that result in them not getting found during the tests (or cause the same problems mentioned for noinst_LTLIBRARIES)?

Given https://github.com/Unidata/netcdf-c/blob/47d35cc24e2ad5d057f98e4ffa9ea522628a7e28/plugins/Makefile.am#L10-L11 https://github.com/Unidata/netcdf-c/blob/47d35cc24e2ad5d057f98e4ffa9ea522628a7e28/plugins/Makefile.am#L22-L26 and https://github.com/Unidata/netcdf-c/blob/47d35cc24e2ad5d057f98e4ffa9ea522628a7e28/plugins/Makefile.am#L111 the test-only plugins should be installed under builddir only, not anywhere under prefix, unless DESTDIR puts them in $(DESTDIR)/$(abs_top_builddir).

1. The rpath ... I would worry about what happens with MINGW (and Cygwin), but since the tests passed, I assume it is ok.

MinGW and Cygwin don't have a concept of rpath, so not providing that flag should be fine on those platforms.

[DennisHeimbigner]

Would putting the test plugins in check_LTLIBRARIES instead of plugin_LTLIBRARIES, lib_LTLIBRARIES, or tmp_LTLIBRARIES work, or does that result in them not getting found during the tests (or cause the same problems mentioned for noinst_LTLIBRARIES)?

I am completely lost here. What exactly are you trying to do that is not being done with the current build? Also where does DESTDIR come into it? I can see no reference to it in Makefile.am?

[DWesl]

Would putting the test plugins in check_LTLIBRARIES instead of plugin_LTLIBRARIES, lib_LTLIBRARIES, or tmp_LTLIBRARIES work, or does that result in them not getting found during the tests (or cause the same problems mentioned for noinst_LTLIBRARIES)?

I am completely lost here. What exactly are you trying to do that is not being done with the current build?

This was partially written before your most recent comment and would be an alternative to defining tmpdir and creating the test plugins as tmp_LTLIBRARIES: check_LTLIBRARIES is like noinst_LTLIBRARIES in that it does not install the resulting shared libraries, but does not build them until someone runs make check. There is a comment saying that noinst_LTLIBRARIES does not work with plugin_LTLIBRARIES, but I don't understand why, so check_LTLIBRARIES might work.

I think specifying that the test plugins are not to be installed is a more natural solution than what is done here, but if it doesn't work then it doesn't actually solve anything.

Also where does DESTDIR come into it? I can see no reference to it in Makefile.am?

DESTDIR would not be in Makefile.am; it is for use by packagers to put the installed files in a temporary install hierarchy instead of the final locations

For example, you could configure with ./configure --prefix=/usr, then install with make install DESTDIR=/tmp/pretend_root and the files to install would go in /tmp/pretend_root/usr/, and creating a distribution file looks something like cd /tmp/pretend_root; tar cf /tmp/dist-file-v1.0.tar * (I think tar has options to make the last step simpler, but I don't remember them)

Given that prefix is usually absolute (I think it defaults to /usr/local, and /usr would be where distribution packagers set it), there is an excellent chance that the test plugins would get installed to $(DESTDIR)/$(abs_top_buildir) (something like /tmp/pretend_root/home/me/src/netcdf-c/build). However, it should still be possible to remove these files with install-{data,exec}-hooks (while the *.la files might be covered by install-data, the *.so files should probably be covered by install-exec). The rule might need to be conditioned on whether DESTDIR is defined, but that should be simple:

install-data-hook:
        if test "x$(DESTDIR)" != "x"; then cd $(DESTDIR)/$(tmpdir) && rm -f $(tmp_LTLIBRARIES); fi

install-exec-hook:
        if test "x$(DESTDIR)" != "x"; then cd $(DESTDIR)/$(tmpdir) && rm -f $(tmp_LTLIBRARIES:.la=.so); fi

would probably delete the files (with the current netcdf-c configuration; syntax borrowed from the install-data-hook in this PR), then the packaging scripts should delete the empty directories before creating the archive.

[opoplawski]

I don't want to suppress the generation of plugins - I want them installed in the proper place. Currently they get installed (also) into $(DESTDIR)/$(abs_top_builddir). The proposed hooks in the previous comment might work.

I tried check_LTLIBRARIES first, but that didn't work. I don't quite remember why - possibly that they were build as static convenience libraries instead of shared libs.

[DennisHeimbigner]

$(DESTDIR)/$(abs_top_builddir)

The use of this by the packager is a major problem since abs_top_builddir is itself intended to be an absolute path. I wish noinst_LTLIBRARIES worked when also using plugin_LTLIBRARIES. I too do not know why it fails to create the testing libraries in that case. My suspicion is that it is an autoconf bug.

So, the workflow I want is:

1. create all libraries (testing and plugin) in some local directory.
2. run tests assuming that the local directory chosen in #1 is known or computable.
3. during install, move the plugin libraries (and not the testing libraries) to the plugin directory.

Note that I resist moving the test libraries and then deleting them is 2-fold:

1. They might overwrite existing user created libraries.
2. As I add new testing libraries I have to remember to add them also to the list of installed libraries to be deleted.

Both #1 and #2 are fixable, but irritating.

So can you outline the most desirable installation situation from your point of view? That is, where ideally do you want the plugin libraries to go?

[opoplawski]

I think we all want the same solution. I just don't think it's possible with autotools. You already have the testing libraries marked differently (tmpdir_LTLIBRARIES), this doesn't change that.

[DWesl]

Would providing -module to the _LDFLAGS of the test plugins help them work properly in check_LTLIBRARIES?

EDIT: Just saw AM_LDFLAGS += -module at the top of the file, so that's already done.

[DennisHeimbigner]

I think the solution in this situation (for packagers) is to transfer the responsibility for installing the filter libraries to be the done by the packager. This, I think, entails the following:

1. build netcdf with the --without-plugin-dir option. This will suppress installation of the filters during "make install".
2. have the plugins/Makefile.am export in some fashion a list of the filters that should be installed
3. The packager then uses #2 and does the installation to wherever it desires.

[DWesl]

I managed to convince libtool (well, actually it was mostly automake that needed convincing) into producing shared libraries for plugins mentioned in check_LTLIBRARIES. It turns out the flag used to tell libtool "I want you to make a shared library (of the sort I can dlopen later)" isn't -shared, it's -rpath. For this reason, automake will add -rpath arguments to every library mentioned in a *_LTLIBRARIES variable other than check_LTLIBRARIES, noinst_LTLIBRARIES, and maybe EXTRA_LTLIBRARIES.

Also, specifying, say, lib__nch5noop_la_LDFLAGS overrides AM_LDFLAGS; that is, the definition of lib__nch5noop_la_LDFLAGS should include AM_LDFLAGS if you want automake to pass any of those flags to the linker. This can be relevant if, for example, one is on Cygwin and need -no-undefined in the link line before libtool will deign to actually link the thing.

One other note about building on Cygwin: the variable to link a library into a library is LIBADD; LDADD appears to be only for programs.

So, to solve the problem of the test plugins getting installed by the workflows for building packages for linux distribution repositories and the like, I think one could put the test plugins in check_LTLIBRARIES (instead of tmp_LTLIBRARIES) and add something like -rpath $(abs_builddir) to the LDFLAGS of each test plugin should allow the plugins to be linked into shared libraries (and work as plugins) but not be installed anywhere (even the build tree). Does this fix the original problem?

[DennisHeimbigner]

I was not aware of check_LTLIBRARIES. I think your solution is correct; I will test. I do note that the requirement for -rpath seems to conflict with the desire above to remove it.

[DWesl]

check_LTLIBRARIES is like noinst_LTLIBRARIES, except the libraries don't get built until someone runs make check (noinst targets seem to get built anytime someone runs make). This may mean the SUBDIRS in the top-level Makefile.am need to be rearranged so the plugins get built before the tests that need them. If there is no order that works, putting them in noinst_LTLIBRARIES instead of check_LTLIBRARIES or tmp_LTLIBRARIES and telling people to do ./configure && make && make check instead of ./configure && make check should work.

automake provides its own -rpath option to libtool for all _LTLIBRARIES targets except check_LTLIBRARIES, so keeping the -rpath in AM_LDFLAGS gave me warnings about including it twice (and libtool seems to just ignore the path given on Windows and Cygwin, using it only to tell "real" libraries from collections of object files you don't want to keep listing out ("convenience libraries"), so the linker doesn't complain).

[DennisHeimbigner]

If I put the plugin dir ahead of all the test directories, then I think the check libraries should get built at the proper time. I need to do some experimentation.

[DWesl]

It would need to be after liblib so the Windows and Cygwin builds can find liblib/libnetcdf.la but before nc_test4, which is the directory I've found with tests of the plugin system.

Looking at the top-level Makefile.am (line 126), that's already the case, so obviously someone else thought of that before me.

[DennisHeimbigner]

So I did a test in which I used either noinst_LTLIBRARIES or check_LTLIBRARIES instead of tmp_LTLIBRARIES. The problem I saw was that in both cases it built the static library but did not build the shared library. I did this with and without rpath set. I assume you are seeing different, so what are you doing different?

[DWesl]

Is -rpath set in lib__ncmisc_la_LDFLAGS = -version-info 0.0.0 -rpath whatever and friends or just in AM_LDFLAGS up top?

[DennisHeimbigner]

Yes, that was it. It also solves the noinst_ problem so I can use noinst_LTLIBRARIES instead of check_LTLIBRARIES

[DWesl]

I prefer check_ to noinst_ for the test plugins because it's clearer about the purpose, but I don't know the naming conventions here.

As you discovered lib__ncstuff_la_LDFLAGS overrides AM_LDFLAGS, so if you want the stuff in AM_LDFLAGS (for example, one of the flags needed for this to compile on Cygwin and probably Windows) to stay, you have to include it as lib__ncstuff_la_LDFLAGS = $(AM_LDFLAGS) -new-flag

[DennisHeimbigner]

When I run my tests with no -rpath for the installable filters, it fails. See e.g. https://github.com/DennisHeimbigner/netcdf-c/runs/7120535287?check_suite_focus=true

[DWesl]

I suspect that particular failure is because configure defaults to --without-plugin-dir/--with-plugin-dir=no, so removing the conditional on whether there actually is a plugin dir confuses things. Does that test case pass if you change set -rpath to /tmp if ENABLE_PLUGIN_DIR is false?

[DennisHeimbigner]

This should be closed in favor of PR https://github.com/Unidata/netcdf-c/pull/2440

[opoplawski]

I've reduced this now to the last remaining item that wasn't addressed - removing to soname from the plugins.

[WardF]

Merged in with #2826