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

Add Cygwin CI and stop installing unwanted plugins #2529

Merged
merged 25 commits into from
Nov 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
844480a
DEV: Add autools-generated files to .gitignore.
DWesl Jun 29, 2022
a69308e
CI: Add Cygwin CI run.
DWesl Jun 29, 2022
086eed0
CI, TST: Check that test plugins don't get installed with DESTDIR
DWesl Jun 30, 2022
0eed60a
BLD: Get netCDF4 build working on Windows.
DWesl Jun 30, 2022
927829e
DEV, TST: Move test plugins to check_LTLIBRARIES instead of tmp_LTLIB…
DWesl Jun 30, 2022
f000e15
BLD: Try fixing the export-symbols problem on MinGW
DWesl Jun 30, 2022
1ef000c
CI, DBG: Upload MinGW test logs on failure.
DWesl Jul 1, 2022
5c9722f
TST: Add code for MinGW to findplugin.sh.
DWesl Jul 1, 2022
c805b02
TST: Mark tst_filter.sh XFAIL on MinGW
DWesl Jul 5, 2022
d33e315
CI: Try to skip failing NCZarr Plugin tests on MinGW
DWesl Jul 5, 2022
a71c606
TST: Mark NCZarr plugins XFAIL on MinGW
DWesl Jul 5, 2022
8b600d3
TST: tst_h_strbug and tst_h_refs pass on Cygwin now
DWesl Jul 5, 2022
543e27c
BLD: Use host platform instead of build platform for platform-specifi…
DWesl Jul 10, 2022
b85c929
CI: Prep Cygwin CI run for adding CMake build.
DWesl Jul 11, 2022
adde7f5
BLD: Get CMake build compiling on Cygwin.
DWesl Jul 11, 2022
a6d0bcc
BUG: Robustify nulldup definition.
DWesl Jul 12, 2022
f28dcaa
TST: Mark nczarr s3 cleanup test XFAIL on Cygwin instead of skipping.
DWesl Jul 12, 2022
18b76ba
CI: Disable CMake tests on Cygwin and MinGW.
DWesl Oct 12, 2022
a0aa2b3
CI: Revert Windows CI run on push.
DWesl Oct 13, 2022
4046afd
BLD: Fix syntax in configure.ac.
DWesl Oct 13, 2022
1ed8ab1
BLD: Avoid specifying -version-info and -avoid-version
DWesl Oct 15, 2022
fb02ff8
BLD: Specify -avoid-version on MinGW and Cygwin; -version-info otherwise
DWesl Oct 27, 2022
4ef6874
STY: Move nulldup backup definition from cp_win32.c to ncconfigure.h
DWesl Oct 29, 2022
3b74e0b
FIX: ifndef requires no parentheses.
DWesl Oct 29, 2022
4c1a39b
BLD: Declare nulldup backup definition static not extern
DWesl Nov 1, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions .github/workflows/run_tests_win_cygwin.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
name: Run Cygwin-based tests

on: [pull_request, workflow_dispatch]

env:
SHELLOPTS: igncr
CHERE_INVOKING: 1
CYGWIN_NOWINPATH: 1

jobs:
build-and-test-autotools:
runs-on: windows-latest
defaults:
run:
shell: bash -eo pipefail -o igncr "{0}"

name: Cygwin-based Autotools tests

steps:
- name: Fix line endings
run: git config --global core.autocrlf input

- uses: actions/checkout@v2

- uses: cygwin/cygwin-install-action@v2
with:
platform: x86_64
packages: >-
git automake libtool autoconf2.5 make libhdf5-devel
libhdf4-devel zipinfo libxml2-devel perl zlib-devel
libzstd-devel libbz2-devel libaec-devel libzip-devel
libdeflate-devel gcc-core

- name: (Autotools) Run autoconf and friends
run: |
cp -f /bin/dash /bin/sh
mkdir m4
/bin/dash /usr/bin/libtoolize --force --copy --verbose
/usr/bin/autoreconf-2.69 --force --install --verbose --debug

- name: (Autotools) Configure in-tree build
run: >-
/bin/dash ./configure --enable-hdf5 --enable-shared
--disable-static --enable-dap --disable-dap-remote-tests
--enable-plugins --disable-nczarr-filters
--disable-nczarr-s3 --disable-nczarr-s3-tests --disable-nczarr

- name: Look at config.log if error
if: ${{ failure() }}
run: cat config.log

- name: Print summary
run: cat libnetcdf.settings

- name: (Autotools) Build library and utilities
run: make -j8 SHELL=/bin/dash

- name: (Autotools) Test DESTDIR install
run: |
make install DESTDIR=/tmp/pretend-root SHELL=/bin/dash
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

- name: (Autotools) Build and run tests
timeout-minutes: 30
run: make check -j8 SHELL=/bin/dash
25 changes: 21 additions & 4 deletions .github/workflows/run_tests_win_mingw.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@

name: Run MSYS2, MinGW64-based Tests

env:
CPPFLAGS: "-D_BSD_SOURCE"

on: [pull_request, workflow_dispatch]
DWesl marked this conversation as resolved.
Show resolved Hide resolved

jobs:

build-and-test:
build-and-test-autotools:

runs-on: windows-latest
defaults:
Expand All @@ -35,7 +37,7 @@ jobs:
run: autoreconf -if

- name: (Autotools) Configure Build
run: ./configure --enable-hdf5 --enable-dap --disable-dap-remote-tests --disable-static --disable-plugins --disable-byterange --disable-dap-remote-tests --disable-logging
run: ./configure --enable-hdf5 --enable-dap --disable-dap-remote-tests --disable-static --disable-byterange --disable-dap-remote-tests --disable-logging --enable-plugins --disable-nczarr-filters --disable-nczarr-s3 --disable-nczarr-s3-tests
if: ${{ success() }}

- name: (Autotools) Look at config.log if error
Expand All @@ -46,9 +48,24 @@ jobs:
run: cat libnetcdf.settings

- name: (Autotools) Build Library and Utilities
run: make -j 8 LDFLAGS="-no-undefined -Wl,--export-all-symbols"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the -no-undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

run: make -j 8 LDFLAGS="-Wl,--export-all-symbols"
if: ${{ success() }}

- name: Check for plugins
run: |
dir ./plugins
dir ./plugins/.libs

- name: (Autotools) Build and Run Tests
run: make check -j 8
run: make check -j 8 LDFLAGS="-Wl,--export-all-symbols"
if: ${{ success() }}
id: tests

- name: Upload test failures
if: ${{ failure() && steps.tests.conclusion == 'failure' }}
uses: actions/upload-artifact@v3
with:
name: mingw-autotools-test-logs
path: |
*/*.log
*/*.trs
14 changes: 14 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,20 @@ dap4_test/findtestserver4.c
dap4_test/pingurl4.c
ncdap_test/findtestserver.c

Makefile.in
aclocal.m4
compile
config.guess
config.h.in
config.sub
configure
depcomp
install-sh
ltmain.sh
m4
missing
test-driver
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@DWesl DWesl Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#####
# End ignored generated files.
#####
Expand Down
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ FILE(COPY ${CMAKE_CURRENT_SOURCE_DIR}/CTestCustom.cmake DESTINATION ${CMAKE_CURR

# Set Memory test program for non-MSVC based builds.
# Assume valgrind for now.
IF((NOT MSVC) AND (NOT MINGW))
IF((NOT MSVC) AND (NOT MINGW) AND (NOT ISCYGWIN))
SET(CTEST_MEMORYCHECK_COMMAND valgrind CACHE STRING "")
ENDIF()

Expand Down
12 changes: 7 additions & 5 deletions RELEASE_NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -277,11 +277,13 @@ See [GitHub #1251](https://github.com/Unidata/netcdf-c/issues/1251).
### 4.5.0-rc1 - June 5, 2017

* [Enhancement] DAP4 is now included. Since dap2 is the default for urls, dap4 must be specified by
(1) using "dap4:" as the url protocol, or
(2) appending "#protocol=dap4" to the end of the url, or
(3) appending "#dap4" to the end of the url
Note that dap4 is enabled by default but remote-testing is
disabled until the testserver situation is resolved.
1. using "dap4:" as the url protocol, or
2. appending "\#protocol=dap4" to the end of the url, or
3. appending "\#dap4" to the end of the url

Note that dap4 is enabled by default but remote-testing is
disabled until the testserver situation is resolved.

* [Enhancement] The remote testing server can now be specified with the `--with-testserver` option to ./configure.
* [Enhancement] Modified netCDF4 to use ASCII for NC_CHAR. See [Github Pull request #316](https://github.com/Unidata/netcdf-c/pull/316) for more information.
* [Bug Fix] Corrected an error with how dimsizes might be read. See [Github #410](https://github.com/unidata/netcdf-c/issues/410) for more information.
Expand Down
18 changes: 10 additions & 8 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -102,21 +102,23 @@ AM_INIT_AUTOMAKE([foreign dist-zip subdir-objects])
AC_CONFIG_SRCDIR([include/netcdf.h])

# Figure out platforms of special interest
case "`uname`" in
CYGWIN*) ISCYGWIN=yes;;
Darwin*) ISOSX=yes;;
WIN*) ISMSVC=yes;;
MINGW*) ISMINGW=yes;;
MSYS*) ISMINGW=yes;;
esac
AC_CANONICAL_HOST
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

AS_CASE([$host],
[*-*-cygwin], [ISCYGWIN=yes],
[*-*-darwin*], [ISOSX=yes],
[*-*-mingw*], [ISMINGW=yes],
[*-*-msys], [ISMINGW=yes],
[*-*-win*], [ISMSVC=yes],
[]
)

if test "x$MSYSTEM" != x ; then
ISMINGW=yes
ISMSYS=yes
fi

# Get windows version info
if text "x$ISMSVC" = xyes ; then
if test "x$ISMSVC" = xyes ; then
WINVER=`systeminfo | sed -e '/^OS Version:/p' -ed | sed -e 's|[^0-9]*\([0-9.]*\).*|\1|'`
else
WINVER="0.0.0"
Expand Down
12 changes: 6 additions & 6 deletions docs/filters.md
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ This means that some mechanism is needed to translate between the HDF5 id+parame
3. It must be possible to modify the set of visible parameters in response to environment information such as the type of the associated variable; this is required to mimic the corresponding HDF5 capability.
4. It must be possible to use filters even if HDF5 support is disabled.

Note that the term "visible parameters" is used here to refer to the parameters provided by "nc_def_var_filter" or those stored in the dataset's metadata as provided by the JSON codec. The term "working parameters" refers to the parameters given to the compressor itself and derived from the visible parameters.
Note that the term "visible parameters" is used here to refer to the parameters provided by `nc_def_var_filter` or those stored in the dataset's metadata as provided by the JSON codec. The term "working parameters" refers to the parameters given to the compressor itself and derived from the visible parameters.

The standard authority for defining Zarr filters is the list supported by the NumCodecs project [7].
Comparing the set of standard filters (aka codecs) defined by NumCodecs to the set of standard filters defined by HDF5 [3], it can be seen that the two sets overlap, but each has filters not defined by the other.
Expand Down Expand Up @@ -514,8 +514,8 @@ This interrogation operates by seeing if certain well-known (function) names are

There will be two library types:

1. HDF5 — exports a specific API: "H5Z_plugin_type" and "H5Z_get_plugin_info".
2. Codec — exports a specific API: "NCZ_get_codec_info"
1. HDF5 — exports a specific API: `H5Z_plugin_type` and `H5Z_get_plugin_info`.
2. Codec — exports a specific API: `NCZ_get_codec_info`

Note that a given library can export either or both of these APIs.
This means that we can have three types of libraries:
Expand Down Expand Up @@ -605,7 +605,7 @@ is stored in the JSON dictionary form described earlier.

The Codec style, using JSON, has the ability to provide very complex parameters that may be hard to encode as a vector of unsigned integers.
It might be desirable to consider exporting a JSON-base API out of the netcdf-c API to support user access to this complexity.
This would mean providing some alternate version of "nc_def_var_filter" that takes a string-valued argument instead of a vector of unsigned ints.
This would mean providing some alternate version of `nc_def_var_filter` that takes a string-valued argument instead of a vector of unsigned ints.
This extension is unlikely to be implemented until a compelling use-case is encountered.

One bad side-effect of this is that we then may have two classes of plugins.
Expand Down Expand Up @@ -812,7 +812,7 @@ The h5 tag indicates that they assume that the result of the parse is a set of u
* idp will contain the first constant — the filter id
* nparamsp will contain the number of params
* paramsp will contain a vector of params — the caller must free
This function can parse single filter spec strings as defined in the section on \ref filters_syntax.
This function can parse single filter spec strings as defined in the section on [Filter Specification Syntax](#filters_syntax).
2. *int ncaux\_h5filterspec\_parselist(const char* txt, int* formatp, size\_t* nspecsp, struct NC\_H5\_Filterspec*** vectorp);*
* txt contains the text of a sequence '|' separated filter specs.
* formatp currently always returns 0.
Expand Down Expand Up @@ -852,7 +852,7 @@ The include file *netcdf\_meta.h* contains the following definition.
````
#define NC_HAS_MULTIFILTERS 1
````
This, in conjunction with the error code *NC\_ENOFILTER* in *netcdf.h* can be used to see what filter mechanism is in place as described in the section on \ref filters_compatibility.
This, in conjunction with the error code *NC\_ENOFILTER* in *netcdf.h* can be used to see what filter mechanism is in place as described in the section on [incompatibities](#filters_compatibility).

1. !defined(NC\_ENOFILTER) && !defined(NC\_HAS\_MULTIFILTERS) — indicates that the old pre-4.7.4 mechanism is in place.
It does not support multiple filters.
Expand Down
Loading