Add Cygwin CI and stop installing unwanted plugins

[DWesl]

This adds a CI job for my development platform, which will hopefully avoid issues like #2196 and #2446.

This also avoids installing test plugins (or any plugins if --without-plugin-dir is passed to configure), which is especially noticed by distribution package managers using a DESTDIR install: that issue was previously raised as #2431.

I tried to get CMake CI runs working on Cygwin and MinGW, but couldn't get any tests to pass on Cygwin or complete the build on MinGW; this is likely due to the problems highlighted in #2290.

Includes cross-compilation suggestion from this comment.

Also includes a few markdown cleanups that seem to have been fixed in other ways since I started working; let me know if you want to approach those changes differently

Replaces #2431; replaces #2440
Might close #2468
Might replace #2479
Might close #2396

[CLAassistant]

All committers have signed the CLA.

[DennisHeimbigner]

This PR will take some time to digest,
but thanks for doing this.

[DennisHeimbigner]

Why remove the -no-undefined?

[DWesl]

It's already in the Makefile.am, and I like having the flags necessary for things to build actually specified by the build system.

I tried removing -Wl,--export-all-symbols as well, but that led to a number of "unresolved reference to symbol nc..." errors at link time. I might be able to make more progress by specifying AM_CPPFLAGS=-DDLL_EXPORT -DDLL_NETCDF in the top-level Makefile.am so the source files properly specify __declspec(dllexport), but given that CMake fails to link I'm not sure that would be enough.

[DennisHeimbigner]

Are you sure that test--driver and ltmain.sh should be ignored?

[DWesl]

For the developers' git repository? Almost certainly. For release tarballs (made with make dist or make distcheck), no, they should absolutely be included.

On a related note, what is the policy on building GitHub's automatic repository tarballs? Do you assume everyone has a full Autotools or CMake install (the assumption I made here) or only POSIX Make and Bourne Shell (the assumption made for Autotools release tarballs)?

[WardF]

We turn on maintainer mode in the release branches, so full installs should not be required. A lot of the generated files are explicitly kept out of version tracking, and only added to the 'v[xyz]-release' branch which gets tagged.

[DWesl]

This will help keep the generated files out of version tracking, but will make adding them to the release branches a bit trickier unless the release branches drop this part of the .gitignore file; should I add a comment at the start to that effect?

E.g.

# Autotools-generated files
# Delete this section on release branches

[DennisHeimbigner]

I vaguely recall that we had problems in the past using AC_CANONICAL_HOST.
Wish I could remember the details.

[DWesl]

It won't auto-discover MSVC, but it should help with #2396, since the current assumption made in using uname directly is that nobody will want to cross-compile at any time for any reason

[DennisHeimbigner]

Not sure what is the point of this change. Also, I believe we have tried to
avoid use of inline because older versions of some C compilers do not support it.

[DWesl]

The point of this change is defining a fallback definition of nulldup in case strdup is not provided by the C runtime. I don't remember if that was a problem on Cygwin or MinGW (EDIT: given the commit, I suspect this is a MinGW problem, not a Cygwin problem), but the current definitions all assume that strdup will be provided if nulldup isn't.

[DWesl]

I tried adding inline, but since it wasn't in the declaration (or the other twenty definitions) the C compiler rejected it and I took it out of the definition.

[DennisHeimbigner]

Good point about strdup; you might consider putting the body of the
alternate nulldup into a more general location such as libdispatch/dutil.c
or putting it into ncconfigure.h as a static function.

[DWesl]

Moved to include/ncconfigure.h

[DennisHeimbigner]

Have you tested this on the platforms we support?

[DWesl]

I've tested this on Cygwin and MinGW on my fork and locally.

I think you've also tested this, since this is basically the solution proposed in #2440.

I have a test for what this is trying to do (fix #2431) in the Cygwin CI (make install DESTDIR=/tmp/pretend-root && test ! -d /tmp/pretend-root/$(pwd), more or less); I could try to add a similar test to the installcheck target, but that might take a while.

[DennisHeimbigner]

OK, I think this is ok to merge subject to testing with Visual Studio.
[Update tested with visual studio (with filters off) and all tests passed]

[DWesl]

Do you want me to try to add MSVC testing to CI as part of this PR? CMake should be straightforward; Autotools will need help (specifically, someone telling me what values to specify for CC, CPP, and LD)

[DennisHeimbigner]

I do note that the Mac build is currently failing for filters.

[DWesl]

Apparently -version-info is required for plugins on MacOS and breaks plugins on MinGW and Cygwin, which is not what I expected

[WardF]

Reviewing extant PR's to pull into v4.9.1-rc2; thanks a lot, I just noticed the checks hadn't run. Running now.

[WardF]

@DennisHeimbigner checks are passing, but you are more plugged into the cygwin ecosystem. Any blocking issues I'm not seeing, here? Otherwise I'll merge it later this afternoon :).

[DennisHeimbigner]

I think my previous review holds, so it should be ok to merge it.

[WardF]

Thanks @DennisHeimbigner !

[DennisHeimbigner]

I am revisiting this because it is causing some problems.
Specifically, I do not understand why github action run_tests_cygin.yml contains the code below.
What is its purpose?

if [ -d "/tmp/pretend-root/$(pwd)" ];
then
find /tmp/pretend-root/$(pwd)
if [ $(find /tmp/pretend-root/$(pwd) -type f | wc -l) -gt 0 ]; then exit 1; fi
fi

[DWesl]

The original issue prompting this was that the test plugins were getting installed, reproducing the source directory in the install directory. This makes sure that neither the source directory nor any of its subdirectories gets installed. I'll look into the problems.

[DWesl]

I am revisiting this because it is causing some problems.

Could you elaborate on what problems you run into? I'm not finding any cases where only the Cygwin workflow fails, only ones that also fail on Ubuntu.

[DennisHeimbigner]

The problem is that noinst causes my test filters to not get built at test time, hence those tests fail.
I am modifying the Makefile.am to use test_LTLIBRARIES and testdir instead. But am running up against
the above piece of code during github action execution.

[DWesl]

Found it here:
main...DennisHeimbigner:netcdf-c:s3lib.tmp#diff-72bb278809ac7dbe0329f064d73cbe73a5dda1d45ea85c288f5b9d3f3cbc07fbL39-R43
you tell Automake you do not have any test libraries, no libraries you don't want to install, only libraries you want to install to the source directory. Here
main...DennisHeimbigner:netcdf-c:s3lib.tmp#diff-72bb278809ac7dbe0329f064d73cbe73a5dda1d45ea85c288f5b9d3f3cbc07fbL123-R125
you tell Automake that you do, in fact, want to install the test plugins, to a rather odd installation directory under what would be the source directory if it were installed at root on the same computer.

The test is checking whether Autotools is installing anything to that rather odd installation directory, and fails, as it is supposed to, when it finds that Autotools has installed the test plugins to that directory, rather than not installing them at all, as you likely intended.

The test is there because many distribution package managers use DESTDIR to install to an empty directory so that creating an archive to install on another computer looks like cd ${DESTDIR}; tar caf ../netcdf-c-${VERSION}.tar.gz *. They would rather not install the test plugins, not least because the people who install the package from their archive would not have the rest of the files to run the tests.

When you change from a target that installs to one that doesn't (i.e. check_LTLIBRARIES, which runs at make check time, or noinst_LTLIBRARIES, which runs at make time), you will need to add -rpath /tmp or similar to the _LDFLAGS of the test plugins, otherwise Autotools will assume the library is for linking into a test executable and not bother creating a shared library (after all, the flag for saying "I want a shared library I can dlopen later" is -rpath rather than -shared, because that isn't the least bit confusung to anyone)

[DWesl]

The problem is that noinst causes my test filters to not get built at test time, hence those tests fail.

That is very odd, since noinst_LTLIBRARIES builds at the same time as everything else (i.e. when you run make). I can't see any reason that wouldn't run before make check, though you could try check_LTLIBRARIES, which would build the library before running the tests.

Can I see the test logs where noinst_LTLIBRARIES fails?

[DennisHeimbigner]

Nevertheless. It does not work.

[DennisHeimbigner]

If there was some way to force DESTDIR to not be applied to the testdir, then things would work ok, I think.

[DWesl]

Nevertheless. It does not work.

The only thing I can think of is maybe someone added new test plugins and forgot to add -rpath /tmp to their _LDFLAGS, so Autotools just build a static library, which the test couldn't dlopen.

It would be more convenient if Autotools had a sensible flag for to tell it we're building a test plugin, rather than it deciding -rpath is the flag to use to tell it "I want to build something I can dlopen later, not just something I can link to a test executable".

[DennisHeimbigner]

AFAIK using your solution, the test filters never get built at make check time in any form, static or dynamic.

[DWesl]

AFAIK using your solution, the test filters never get built at make check time in any form, static or dynamic.

That is very odd. I'd like to check this; where are the plugin tests that are failing? I put all test plugins in check_LTLIBRARIES. I built and ran the tests locally when creating this PR, so those should be passing.

The other option is reverting this PR and using the original version of #2431, allowing Autotools to install the files to wherever and then removing them right after. /tmp/testplugins might be a convenient install directory for the test plugins, since the tests use the versions in the current directory.

I still like the "don't install the test plugins in the first place" option, which is why I made this PR. It did take a fair bit of digging (discussion around this from here to here to find out that just adding the test libraries to check_LTLIBRARIES wouldn't build the shared libraries the tests need, since the tests dlopen the plugins rather than linking to them; you need to add -rpath to the corresponding lib__plugin_name_la_LDFLAGS variables, which is done automatically for all _LTLIBRARIES variables except check_LTLIBRARIES and noinst_LTLIBRARIES.

[DWesl]

I'm attempting to reproduce this with your branch s3lib.tmp, and the following configure line:

../configure --enable-doxygen --enable-dot --disable-static --enable-shared -C --no-create --no-recursion --enable-s3 --enable-s3-internal --enable-nczarr-s3-tests --enable-nczarr --enable-nczarr-filters --enable-byterange --enable-hdf5 --enable-dap --enable-dap-remote-tests --enable-logging

The first problem I run into is that something in libdispatch uses SSL functions directly, and appears to assume that linking against libcurl will bring those functions in at runtime, and all will be well. However, this breaks the assumptions of the -no-undefined libtool flag, which states that all functions will be resolvable at link time and is required on Windows (and therefore Cygwin).

I have jury-rigged shims in there that should let me compile, but I am unsure how to do this properly or generally; perhaps Autoconf Archive's ax_check_openssl would help, at least for platforms that are neither Windows or Apple?

[DennisHeimbigner]

I fixed this in PR #2658.

[DWesl]

It looks like I forgot to test with ENABLE_PLUGIN_DIR false/--without-plugin-dir; should I add something like that to the Cygwin CI workflow?

[DennisHeimbigner]

Good idea.

[DWesl]

Now #2659.

Looks like I was wrong. Still no idea what was wrong or why that fixed things.