-
Notifications
You must be signed in to change notification settings - Fork 262
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
Further simplify the plugin installation process. #2440
Conversation
re: Unidata#2431 Hat tip to [DWesl](https://github.com/DWesl) and [opoplawski](https://github.com/opoplawski) for their help. With some help, I found out how to get rid of the tmp_LTLIBARIES hack and replace it with check_LTLIBRARIES. Using check_LTLIBRARIES means also that the test libraries are not built until "make check" is executed. This PR replaces the one above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth adding a step checking make DESTDIR=/tmp/pretend-root install
doesn't install any of the test plugins to one of the CI jobs?
if ! find /tmp/pretend-root -name lib__nctestplugin.la; exit 1
might work as the condition.
plugins/Makefile.am
Outdated
plugindir = ${ALTPLUGINDIR} | ||
endif | ||
#else | ||
plugindir = ${prefix} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen anything tying --enable-plugins
to --with-plugin-dir
; I think just specifying --enable-plugins
without --with-plugin-dir
will put the (non-test) plugins into ${prefix}
(test run with the old ALTPLUGINDIR
value here). Would ${libexecdir}/netcdf-c
or similar make more sense for the fallback location, would a better fallback be not installing those plugins at all (as implied by the old value), or is there some reasoning for this choice I'm not aware of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New version is basically what I ended up with.
lib__nczhdf5filters_la_SOURCES = NCZhdf5filters.c | ||
|
||
lib__nczhdf5filters_la_LDFLAGS = ${INSTALLFLAGS} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You seem to have commented out the INSTALLFLAGS
assignment on line 20; should the _LDFLAGS
lines with just that be deleted (allowing the default AM_LDFLAGS
to serve those plugins, or the INSTALLFLAGS
assignment uncommented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uncommented the INSTALLFLAGS
definition and started it with AM_LDFLAGS
, that'll help Cygwin builds (and probably Windows).
Doesn't look like there's a "resolve" button like I've seen on some of these.
* If --disable-plugin is set, then force --without-plugin-dir * Propagate changes to CMakeLists.txt * Fix a couple of memory leaks in nc4internal.c and dhttp.c I was unable to figure out a way to prevent any attempt to install the shared libraries when --without-plugin-dir is used. It turns out you can conditionally set the install library, but it seems to ignore the conditional so in that case plugindir needs to be set to some internal location in the build tree. In any case, you still need a location for use with -rpath in order to force creation of the .so files.
This seems to work for me. Thanks. |
@DennisHeimbigner some conflicts have crept in, and Github is (again) not letting me resolve them. I'll try to sort that out, but are you able to make changes and/or have a moment to address them? |
Ok, let me see what I can do. |
This PR was apparently made moot by other, later PRs. |
re: #2431
Hat tip to DWesl and opoplawski for their help.
With some help, I found out how to get rid of the tmp_LTLIBARIES
hack and replace it with check_LTLIBRARIES.
Using check_LTLIBRARIES means also that the test libraries are not
built until "make check" is executed.
This PR replaces the one above.