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

Further simplify the plugin installation process. #2440

Closed

Conversation

DennisHeimbigner
Copy link
Collaborator

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.

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.
Copy link
Contributor

@DWesl DWesl left a 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.

plugindir = ${ALTPLUGINDIR}
endif
#else
plugindir = ${prefix}
Copy link
Contributor

@DWesl DWesl Jun 30, 2022

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?

Copy link
Contributor

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}
Copy link
Contributor

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?

Copy link
Contributor

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.

@WardF WardF added this to the 4.9.1 milestone Jul 1, 2022
* 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.
@opoplawski
Copy link
Contributor

This seems to work for me. Thanks.

@WardF
Copy link
Member

WardF commented Sep 14, 2022

@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?

@DennisHeimbigner
Copy link
Collaborator Author

Ok, let me see what I can do.

@DennisHeimbigner
Copy link
Collaborator Author

This PR was apparently made moot by other, later PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants