@skosukhin Apologies, but clicking "Rebase the PR" to trigger tests run resulted in this mess somehow 😟 So, can you please push the branch again and ping me for a review?

EDIT: otherwise I can also force push on your branch, if you'd like me to.

@skosukhin Fixed it manually, I rebased and push forced to your branch - since permission was granted for it.

@johnwparent Are the problems related to the new version 4.9.2? Does removing it "fixes" the installation on Windows? If so, I can remove the new version from this PR and you can add it in a follow-up PR together with all the required fixes. I would prefer a follow-up PR from you in any case, to be honest, but uploading patches here or making suggestions in the UI will do as well.

@skosukhin No, not all the problems are related to the new version, although I can't say which are because I can't get through a single build of any version right now. The current issues seem related to the new variants/dependencies you've added. The first is that the CMake builder was not updated with the appropriate cmake args to handle the byterange variant (which is the patch I have locally). The issue I'm currently running into is that there is a mix of MSVC runtime libraries introduced by the linkage between netcdfc and bzip2, which is preventing me from building any version and testing between 4.9.2 and 4.8.1. This may be a bzip2 problem as well, but it seems as though it's netcdf that's bringing in the static runtime during a shared link, although perhaps the blame for that falls on bzip2 for linking to the shared runtime as a static library.

A quick solution for bzip2 could be to make bzip2 a dependency on non-Windows platforms only and use the vendored one on Windows for the time being.

A quick solution for bzip2 could be to make bzip2 a dependency on non-Windows platforms only and use the vendored one on Windows for the time being.

I'm more than happy to remove the dep on bzip2 for Windows for the time being until I have more time to sort that out, but that means disabling the use of bzip2 in netcdf-c entirely for Windows as there is no guarantee that the package will be available to the build. Windows does not vendor compression utilities, only tar.
In general, from what I understand, Spack should not be relying on a dependency "just being available on the system" arbitrarily without some sort of modeling/representation in Spack, and from a maintenance standpoint, that feels incredibly fragile/will lead to headaches/issues.

Windows does not vendor compression utilities

But Netcdf-c does.

Windows does not vendor compression utilities

But Netcdf-c does.

Nice, very useful! I wish more libraries did this, would make my life on Windows far easier. In that case, yeah, lets disable an external bzip2 for Windows, but can we get some modeling in Spack indicating that we're using this? For example VTK/Paraview/CMake have a variant for the use of the dependencies they also vendor, as perhaps it would also be useful on unix to have this.

Something like:

variant("internal_bzip2", default=sys.platform == "win32")
variant("internal_<some other dep>", ...)

The usage of vendored libraries that are already spackaged is discouraged as well as the variants that enable it. And I agree with that.

UPD: However, I'm fine with using the vendored bzip2 on Windows as a temporary workaround.

And I'm sorry but in my opinion, by making the decision to build libraries from sources on Windows you chose against having easy life :)

The usage of vendored libraries that are already spackaged is discouraged as well as the variants that enable it. And I agree with that.

There is already precedent for exceptions to this idiom for reasons that parallel this one. Don't define the other variants, but if we're making this choice IMO it should be clear to the users. Make the variant only available on Windows if need be, but if builds fails because of a bz2 it should be immediately clear where it's coming from as the default assumption would generally be a spack derived bz2. Maybe I'm completely off base here.

FWIW I'm likely just going to contribute a CMake system for bz2 as the issues are fairly fundamental (there's no shared option on Windows right now whatsoever) so this fix is going to have to hold up for a while.

Edit:

And I'm sorry but in my opinion, by making the decision to build libraries from sources on Windows you chose against having easy life :)

Someone has to build from source to produce the binaries for everyone else, no reason why that process shouldn't be reproduceable...

Make the variant only available on Windows if need be

If we need a variant, let's make it Windows only. In general, I'd avoid to have platform specific variants creep into other platforms.

Make the variant only available on Windows if need be

If we need a variant, let's make it Windows only. In general, I'd avoid to have platform specific variants creep into other platforms.

Makes sense/I agree.

@johnwparent please, check whether it works now.

As for the platform-specific variant that enables the usage of a vendored library, I don't agree with that but I'm not the one with the merge button here :)

@johnwparent please, check whether it works now.

As for the platform-specific variant that enables the usage of a vendored library, I don't agree with that but I'm not the one with the merge button here :)

Running the build now.

I also do not have any merge power so I guess someone else gets to make that decision for us! Sorry @alalazo.

@skosukhin The build itself succeeds for both the new version added by this PR and the old version! However the compiler wrapper filters fail on Windows with the stack trace:

  File "C:\spack_win\spack\lib\spack\spack\mixins.py", line 104, in _filter_compiler_wrappers_impl
    x.filter(wrapper_path, compiler_path, **filter_kwargs)
  File "C:\spack_win\spack\lib\spack\llnl\util\filesystem.py", line 421, in filter
    return filter_file(
  File "C:\spack_win\spack\lib\spack\llnl\util\filesystem.py", line 324, in filter_file
    unescaped = repl.replace(r"\\", "\\")
AttributeError: 'NoneType' object has no attribute 'replace'

FWIW I disabled the compiler wrapper filter on the HDF5 package, so we may want to do something like that here. We don't use the Spack compiler wrappers on Windows at the moment anyway, so it's a no-op either way.

@johnwparent Fine with you to merge this, and have a follow up later?

@johnwparent Fine with you to merge this, and have a follow up later?

I'm on mobile at the moment so I can't tell if my compiler wrappers comments have been addressed. If they have, yes, LGTM!
Otherwise, considering that it breaks the package for windows users no, we should not merge this.

@johnwparent I don't see changes to filter_compiler_wrappers in this PR. The last change was in #36825 which effectively fixed a bug that was introduced in #34935 and broke the build on Linux.

So to summarize:

1. If current develop works on Windows and this breaks on filter_compiler_wrappers I'd like to investigate why.
2. Otherwise I'd just merge this, since it doesn't touch this aspect.

In case we can make filter_compiler_wrapper a no-op on Windows in a small follow up PR. Let me know if you have more details on the failure on Windows.

Looks like netcdf-c is already broken in develop (yet another argument for getting the Windows CI on it's feet) so this PR looks great, apologies for the false alarm, as you can see it's bit of a can of worms to keep things functional and stable on Windows at the moment.

LGTM!

@skosukhin - apologies if this isn't the correct place to ask this... Is there a reason you no longer link to libcurl when +dap (but instead only provide library search dirs)? This change seems to be breaking my builds of netcdf-c when using +mpi. I get the following config error, which suggests contamination from the system libs:

configure:4614: checking whether we are cross compiling
configure:4622: /opt/cray/pe/mpich/8.1.25/ofi/gnu/9.1/bin/mpicc -o conftest   -L/glade/derecho/scratch/vanderwb/spack-tests/derecho/23.05/envs/build/opt/__spack_path_placeh
older__/__spack_path_placeholder__/_/hdf5/1.12.2/cray-mpich/8.1.25/gcc/12.2.0/lib -L/glade/derecho/scratch/vanderwb/spack-tests/derecho/23.05/envs/build/opt/__spack_path_pl
aceholder__/__spack_path_placeholder__/_/zlib/1.2.13/gcc/7.5.0/lib -L/glade/derecho/scratch/vanderwb/spack-tests/derecho/23.05/envs/build/opt/__spack_path_placeholder__/__s
pack_path_placeholder__/_/libszip/2.1.1/gcc/7.5.0/lib -L/glade/derecho/scratch/vanderwb/spack-tests/derecho/23.05/envs/build/opt/__spack_path_placeholder__/__spack_path_pla
ceholder__/_/bzip2/1.0.8/gcc/12.2.0/lib -L/glade/derecho/scratch/vanderwb/spack-tests/derecho/23.05/envs/build/opt/__spack_path_placeholder__/__spack_path_placeholder__/_/z
std/1.5.5/gcc/7.5.0/lib -L/glade/derecho/scratch/vanderwb/spack-tests/derecho/23.05/envs/build/opt/__spack_path_placeholder__/__spack_path_placeholder__/_/c-blosc/1.21.1/gc
c/12.2.0/lib -L/glade/derecho/scratch/vanderwb/spack-tests/derecho/23.05/envs/build/opt/__spack_path_placeholder__/__spack_path_placeholder__/_/curl/8.0.1/gcc/7.5.0/lib con
ftest.c  >&5
configure:4626: $? = 0
configure:4633: ./conftest
./conftest: symbol lookup error: /usr/lib64/libssh.so.4: undefined symbol: EVP_KDF_CTX_new_id, version OPENSSL_1_1_1d

If I change the package.py as follows, the build works without issue:

@@ -467,7 +467,8 @@ def configure_args(self):
         if self.spec.satisfies("@:4.7~dap+byterange"):
             extra_libs.append(self.spec["curl"].libs)
         elif "+dap" in self.spec or "+byterange" in self.spec:
-            lib_search_dirs.extend(self.spec["curl"].libs.directories)
+            extra_libs.append(self.spec["curl"].libs)