Port netcdf-c to Windows by johnwparent · Pull Request #34935 · spack/spack

johnwparent · 2023-01-13T20:55:15Z

Ports of Netcdf-c to be compatible with Windows - end goal of supporting Paraview on Windows.

spackbot-app · 2023-01-13T20:55:24Z

Hi @johnwparent! I noticed that the following package(s) don't yet have maintainers:

netcdf-cxx

Are you interested in adopting any of these package(s)? If so, simply add the following to the package class:

    maintainers = ["johnwparent"]

If not, could you contact the developers of this package and see if they are interested? You can quickly see who has worked on a package with spack blame:

$ spack blame netcdf-cxx

Thank you for your help! Please don't add maintainers without their consent.

You don't have to be a Spack expert or package developer in order to be a "maintainer," it just gives us a list of users willing to review PRs or debug issues relating to this package. A package can have multiple maintainers; just add a list of GitHub handles of anyone who wants to volunteer.

skosukhin · 2023-01-16T08:34:07Z

I guess it makes sense to reference #33564 here.

zackgalbreath · 2023-01-27T20:51:32Z

@spackbot run pipeline

spackbot-app · 2023-01-27T20:51:38Z

I've started that pipeline for you!

scheibelp · 2023-02-04T02:25:52Z

@spackbot run pipeline

spackbot-app · 2023-02-04T02:25:57Z

I've started that pipeline for you!

scheibelp

I have one question and one request

var/spack/repos/builtin/packages/netcdf-c/package.py

scheibelp · 2023-02-04T03:20:20Z

var/spack/repos/builtin/packages/netcdf-c/netcdfc-hdf5-link-mpi.patch

+
+ IF(MPI_C_INCLUDE_PATH)
+     target_include_directories(netcdf PUBLIC ${MPI_C_INCLUDE_PATH})
+    target_link_libraries(netcdf MPI::MPI_C)


These patches appear to be fundamental - do you know if there are any PRs for netcdf-c adding this functionality?

Sorry, I must have missed this comment last week. Yes, there are, I made a PR implementing the relevant changes in netcdf around when I made this PR. FWIW the include change was not included in my changes as it seems as if there is some debate as to that point between netcdfc and hdf5.

scheibelp

I have a couple requests

var/spack/repos/builtin/packages/netcdf-c/package.py

var/spack/repos/builtin/packages/netcdf-cxx/package.py

scheibelp · 2023-02-15T01:30:10Z

Interestingly, the CI appears to fail legitimately here too, but for paraview (after building netcdf-c and netcdf-cxx):

__spack_path_placeholder__/__spack_path_placeholder__/__spack_path_placeholder__/__spack_path/linux-amzn2-x86_64_v3/gcc-7.3.1/libxml2-2.10.3-g6w7zcj4juhpgptfzeoauubewsgcsvmn/lib/libxml2.so  -lpthread && :
/usr/bin/ld: cannot find -lhdf5_hl-shared
/usr/bin/ld: cannot find -lhdf5-shared
collect2: error: ld returned 1 exit status

Paraview depends on netcdf-c (but not netcdf-cxx) so this would have something to do with locating hdf5 as part of the netcdf-c build: it's possible that the netcdf-c build needs to be updated to locate hdf5, and this in turn helps the paraview build. Alternatively, it may be that paraview needs to be updated to locate hdf5 itself, and whatever prior logic existed in netcdf-c was simply obscuring a mistake in the paraview build.

var/spack/repos/builtin/packages/netcdf-cxx/package.py

johnwparent · 2023-03-08T00:00:08Z

@scheibelp I have determined the origin of the error (Netcdfc's cmake is the culprit) and am in the process of creating yet another patch and upstream PR to netcdfc to fix this.

skosukhin · 2023-03-20T14:19:40Z

var/spack/repos/builtin/packages/netcdf-c/package.py

+        depends_on("autoconf", type="build", when="@4.7.0,main")
+        depends_on("automake", type="build", when="@4.7.0,main")
+        depends_on("libtool", type="build", when="@4.7.0,main")
+        depends_on("m4", type="build")


Are you sure, we don't need m4 when building with cmake (e.g. here)?

netcdf-c master (and any subsequent releases post 4.9.0) will likely need this enabled yes, however for the time being netcdf-c builds perfectly fine without it as the files m4 is looking to generate are checked into source. I'll add the dep back for linux platforms, but as Spack does not currently support installing m4 for Windows, we won't add it as a dep for Windows.

NetCDF-C patches Fixup netcdf HDF5+zlib detection Netcdf's cmake is unable to detect that HDF5 was compiled with zlib Add a patch that allows for said detection netcdf-c patch ill formed cmakelists Add netcdf linker patch

skosukhin · 2023-03-27T12:11:54Z

var/spack/repos/builtin/packages/netcdf-c/4.8.1-win-hdf5-with-zlib.patch

+   # need to know whether to add "-lz" to the symbol
+   # tests below.
+-  CHECK_C_SOURCE_COMPILES("#include <H5public.h>
+  CHECK_C_SOURCE_COMPILES("#include <H5pubconf.h>


@johnwparent This patch reverts Unidata/netcdf-c#1731 and H5public.h does include H5pubconf.h. So, why is it needed on Windows?

I am asking because I am refactoring the netcdf-c package now and figuring out whether we need all the patches we apply.

alalazo · 2023-04-13T12:20:26Z

var/spack/repos/builtin/packages/netcdf-c/package.py

+class BackupStep:
+    @property
+    def _nc_config_backup_dir(self):
+        return join_path(self.pkg.metadata_dir, "spack-nc-config")
+
+    @run_after("install")
+    def backup_nc_config(self):
+        # We expect this to be run before filter_compiler_wrappers:
+        nc_config_file = self.prefix.bin.join("nc-config")
+        if os.path.exists(nc_config_file):
+            mkdirp(self._nc_config_backup_dir)
+            install(nc_config_file, self._nc_config_backup_dir)


If we want to decorate with run_after a method from a class that is not a builder, we need to set explicitly the correct metaclass:

class BackupStep(metaclass=spack.builder.PhaseCallbacksMeta): ...

Is this or any of the other several multi build gotchas documented anywhere that I seem to be missing?
Edit: This is clearly a python language thing rather than a spack thing, but could still use a more obvious callout.

spackbot-app bot added dependencies libraries patch update-package labels Jan 13, 2023

johnwparent requested a review from scheibelp January 13, 2023 20:55

spackbot-app bot requested review from WardF and skosukhin January 13, 2023 20:55

johnwparent linked an issue Jan 13, 2023 that may be closed by this pull request

Paraview Support on Windows #34938

Closed

scheibelp requested changes Feb 4, 2023

View reviewed changes

scheibelp requested changes Feb 10, 2023

View reviewed changes

var/spack/repos/builtin/packages/netcdf-c/package.py Outdated Show resolved Hide resolved

var/spack/repos/builtin/packages/netcdf-c/package.py Show resolved Hide resolved

scheibelp previously approved these changes Feb 10, 2023

View reviewed changes

scheibelp enabled auto-merge (squash) February 10, 2023 23:37

scheibelp reviewed Feb 13, 2023

View reviewed changes

var/spack/repos/builtin/packages/netcdf-cxx/package.py Outdated Show resolved Hide resolved

johnwparent dismissed scheibelp’s stale review via 93271e5 February 14, 2023 20:23

johnwparent commented Feb 16, 2023

View reviewed changes

var/spack/repos/builtin/packages/netcdf-cxx/package.py Outdated Show resolved Hide resolved

alalazo assigned scheibelp Mar 8, 2023

johnwparent force-pushed the win-paraview/port-netdfs branch from 66b7539 to bc079e6 Compare March 14, 2023 15:56

skosukhin reviewed Mar 20, 2023

View reviewed changes

johnwparent added 4 commits March 20, 2023 13:06

netcdf-c

ba87c5d

update netcdf-c

11cfcc0

NetCDF-C patches Fixup netcdf HDF5+zlib detection Netcdf's cmake is unable to detect that HDF5 was compiled with zlib Add a patch that allows for said detection netcdf-c patch ill formed cmakelists Add netcdf linker patch

Proper netcdf import mpi patch

d239cc2

netcdfc: Correct setup_run_environment invocation

75ab27a

johnwparent added 5 commits March 20, 2023 13:06

netcdfc: condesne patches; add dropped cmake args

9420260

Try autotools on unix

e5a2d05

Correct autoreconf invocation

57e797e

Reformat autotools netcdfc builder

b882999

Add m4 back on nix

a7ba778

johnwparent force-pushed the win-paraview/port-netdfs branch from d9c8cf6 to a7ba778 Compare March 20, 2023 17:49

johnwparent changed the title ~~Port netcdf-c[xx] to Windows~~ Port netcdf-c to Windows Mar 20, 2023

scheibelp approved these changes Mar 21, 2023

View reviewed changes

scheibelp merged commit 2e9d0e1 into spack:develop Mar 21, 2023

skosukhin reviewed Mar 27, 2023

View reviewed changes

alalazo reviewed Apr 13, 2023

View reviewed changes

This was referenced Apr 13, 2023

Multiple build systems: second builders do not inherit phase callbacks #36689

Closed

netcdf-c: major refactoring, new variants and versions #36485

Merged

Conversation

johnwparent commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spackbot-app bot commented Jan 13, 2023

Uh oh!

skosukhin commented Jan 16, 2023

Uh oh!

zackgalbreath commented Jan 27, 2023

Uh oh!

spackbot-app bot commented Jan 27, 2023

Uh oh!

scheibelp commented Feb 4, 2023

Uh oh!

spackbot-app bot commented Feb 4, 2023

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scheibelp Feb 4, 2023

Choose a reason for hiding this comment

Uh oh!

johnwparent Feb 10, 2023

Choose a reason for hiding this comment

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scheibelp commented Feb 15, 2023

Uh oh!

Uh oh!

johnwparent commented Mar 8, 2023

Uh oh!

skosukhin Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

johnwparent Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

skosukhin Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

alalazo Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnwparent Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

johnwparent commented Jan 13, 2023 •

edited

Loading

alalazo Apr 13, 2023 •

edited

Loading

johnwparent Apr 17, 2023 •

edited

Loading