Patch #3090 - Fix and test S3 enabled builds

[mannreis]

After #3090 merged I cannot build the main netcdf branch with: cmake ../ -DNETCDF_ENABLE_NCZARR=ON -DNETCDF_ENABLE_S3_INTERNAL=ON

This PR tries to fix the most of it but there are a lot of confusion around: NC_sortenvv, nczm_sortenvv and NC_freeenv vs NCZ_freeenv

@DennisHeimbigner can you please check?

---

Fixes https://github.com/Unidata/netcdf-c/issues/3147

[DennisHeimbigner]

-NC_s3sdkclose(void* s3client0, NCS3INFO* info, int deleteit, char** errmsgp) +NC_s3sdkclose(void* s3client0, char** errmsgp)

I am confused by this PR. For example, what is the reason you are changing the signature of this function?

[mannreis]

@DennisHeimbigner if you compile the current main branch in the following way you'll see the problem:

git clone https://github.com/Unidata/netcdf-c.git
mkdir netcdf-c/build
cd netcdf-c/build
cmake ../ -DNETCDF_ENABLE_NCZARR=ON -DNETCDF_ENABLE_S3_INTERNAL=ON
cmake --build . -j 16

The PR is just to show that the current main is broken when enabling S3.

/home/reis/netcdf-c/libnczarr/zmap_s3sdk.c: In function 'zs3open':
/home/reis/netcdf-c/libnczarr/zmap_s3sdk.c:230:16: warning: implicit declaration of function 'NC_s3sdkgetkeys'; did you mean 'NC_s3sdkdeletekey'? [-Wimplicit-function-declaration]
  230 |     if((stat = NC_s3sdkgetkeys(z3map->s3client,z3map->s3.bucket,z3map->s3.rootkey,&nkeys,NULL,&z3map->errmsg)))
      |                ^~~~~~~~~~~~~~~
      |                NC_s3sdkdeletekey
/home/reis/netcdf-c/libnczarr/zmap_s3sdk.c: In function 'zs3truncate':
/home/reis/netcdf-c/libnczarr/zmap_s3sdk.c:264:47: warning: passing argument 2 of 'NC_s3sdkclose' from incompatible pointer type [-Wincompatible-pointer-types]
  264 |     if(s3client) {stat=NC_s3sdkclose(s3client,&info,1,NULL);}
      |                                               ^~~~~
      |                                               |
      |                                               NCS3INFO *
In file included from /home/reis/netcdf-c/libnczarr/zincludes.h:46,
                 from /home/reis/netcdf-c/libnczarr/zmap_s3sdk.c:6:
/home/reis/netcdf-c/include/ncs3sdk.h:64:51: note: expected 'char **' but argument is of type 'NCS3INFO *'
   64 | EXTERNL int NC_s3sdkclose(void* s3client0, char** errmsgp);
      |                                            ~~~~~~~^~~~~~~
/home/reis/netcdf-c/libnczarr/zmap_s3sdk.c:264:24: error: too many arguments to function 'NC_s3sdkclose'
  264 |     if(s3client) {stat=NC_s3sdkclose(s3client,&info,1,NULL);}
      |                        ^~~~~~~~~~~~~
In file included from /home/reis/netcdf-c/libnczarr/zincludes.h:46,
                 from /home/reis/netcdf-c/libnczarr/zmap_s3sdk.c:6:
/home/reis/netcdf-c/include/ncs3sdk.h:64:13: note: declared here
   64 | EXTERNL int NC_s3sdkclose(void* s3client0, char** errmsgp);
      |             ^~~~~~~~~~~~~
/home/reis/netcdf-c/libnczarr/zmap_s3sdk.c: In function 'zs3close':
/home/reis/netcdf-c/libnczarr/zmap_s3sdk.c:410:40: warning: passing argument 2 of 'NC_s3sdkclose' from incompatible pointer type [-Wincompatible-pointer-types]
  410 |         NC_s3sdkclose(z3map->s3client, &z3map->s3, deleteit, &z3map->errmsg);
      |                                        ^~~~~~~~~~
      |                                        |
      |                                        NCS3INFO *
In file included from /home/reis/netcdf-c/libnczarr/zincludes.h:46,
                 from /home/reis/netcdf-c/libnczarr/zmap_s3sdk.c:6:
/home/reis/netcdf-c/include/ncs3sdk.h:64:51: note: expected 'char **' but argument is of type 'NCS3INFO *'
   64 | EXTERNL int NC_s3sdkclose(void* s3client0, char** errmsgp);
      |                                            ~~~~~~~^~~~~~~
/home/reis/netcdf-c/libnczarr/zmap_s3sdk.c:410:9: error: too many arguments to function 'NC_s3sdkclose'
  410 |         NC_s3sdkclose(z3map->s3client, &z3map->s3, deleteit, &z3map->errmsg);
      |         ^~~~~~~~~~~~~
In file included from /home/reis/netcdf-c/libnczarr/zincludes.h:46,
                 from /home/reis/netcdf-c/libnczarr/zmap_s3sdk.c:6:
/home/reis/netcdf-c/include/ncs3sdk.h:64:13: note: declared here
   64 | EXTERNL int NC_s3sdkclose(void* s3client0, char** errmsgp);
      |             ^~~~~~~~~~~~~
/home/reis/netcdf-c/libnczarr/zmap_s3sdk.c: In function 'zs3search':
/home/reis/netcdf-c/libnczarr/zmap_s3sdk.c:466:44: warning: conversion to 'size_t' {aka 'long unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
  466 |             const char* is = nclistget(tmp,i);
      |                                            ^
/home/reis/netcdf-c/libnczarr/zmap_s3sdk.c:468:52: warning: conversion to 'size_t' {aka 'long unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
  468 |                 const char* js = nclistget(matches,j);
      |                                                    ^
/home/reis/netcdf-c/libnczarr/zmap_s3sdk.c: In function 's3clear':
/home/reis/netcdf-c/libnczarr/zmap_s3sdk.c:508:20: warning: implicit declaration of function 'NC_s3sdksearch'; did you mean 'NC_s3sdkread'? [-Wimplicit-function-declaration]
  508 |         if((stat = NC_s3sdksearch(s3client, bucket, rootkey, &nkeys, &list, NULL)))
      |                    ^~~~~~~~~~~~~~
      |                    NC_s3sdkread
gmake[2]: *** [libnczarr/CMakeFiles/nczarr.dir/build.make:443: libnczarr/CMakeFiles/nczarr.dir/zmap_s3sdk.c.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:3057:

Feel free to close it or add to it.

[mannreis]

Just so that it doesn't go noticed: that both Github actions workflows on windows, cygwin and mingw have explicitly disabled S3 builds. Should we dare changing that?!

[DennisHeimbigner]

We should be able to enable it (using internal s3 sdk) for cygwin. Not sure about mingw.

[DennisHeimbigner]

I am seriously confused about what is going on here.

[WardF]

@mannreis I'm setting up to examine and test locally; for standardization purposes, which version of the s3 sdk are you working against?

[mannreis]

I'm sorry for the noise. I tried to modify the GitHub actions workflows to build with the internal S3 sdk (same as I was working with) but it was a trial and error journey. That's why I squashed commits and force pushed.

[WardF]

@DennisHeimbigner So I've been digging into this, and I think I have an idea of what's going on. Mannreis isn't the one changing the function signature; part of your change with #3090 seems to have made these changes. Because we weren't testing against internal S3 implementation, we failed to catch the compilation errors. That is definitely a mistake on my part, and one we will rectify.

I'm also working to see what it will take to fix this, and I think I have the issue narrowed down. The main thing I'm missing now is the implementation of whatever NC_sortenvv and NC_freeenvv are. Do you have those on a branch on your fork?

[mannreis]

The main thing I'm missing now is the implementation of whatever NC_sortenvv and NC_freeenvv are. Do you have those on a branch on your fork?

I've picked the implementation of those missing functions from https://github.com/DennisHeimbigner/netcdf-c/tree/v3plug.dmh @WardF

The current state of this PR is:

• I resolved the compilation errors and all tests pass on my setup.
• I noticed there is an incoherent behavior for the builds between with autotools and cmake. In autotools one cannot enable s3 without also enabling nczarr. Whereas in cmake they are not conditional. I did not fix this. Simply adjusted the pipeline build switches, not triggering --disable-nczarr --enable-s3 --enable-s3-internal.
• I've enabled ZARR, S3 (internal only) builds and respective tests for ubuntu, osx and cygwin but this last one is failing.

[DennisHeimbigner]

In autotools one cannot enable s3 without also enabling nczarr.

This may be an error on my part. I believe that the HDF5 ROS3 driver uses its own S3 sdk (it is where I obtained the base code for the Internal S3 code). But I think that the byterange code also supports S3, and the byterange code is intended to be independent of NCZarr. You might try enabling byterange and disabling NCZarr and see what happens.

[WardF]

@mannreis Thank you; I'll make the changes so that autotools and cmake are consistent. I will also take a look at the cygwin test to see what we can sort out. Thanks!

[WardF]

I discovered where the missing functions originally were, #3094. Taking a look at this PR to fix the cygwin and build-system issues, and then I will figure out whether or not anything else in #3094 remains relevant.

[WardF]

@mannreis I see you've picked up the implementations as you mentioned, have you pushed those yet? I've reconciled the build systems and am going to look at the Cygwin stuff now, but I want to test my fixes against your latest code if possible. Thanks!

[mannreis]

@mannreis I see you've picked up the implementations as you mentioned, have you pushed those yet?

Yes I did but since you pointed to #3094, maybe it makes more sense to keep the code from it. Sorry for the latency on the replies and thanks the help

[WardF]

Let me take a look and see what happens with #3094

[WardF]

I believe I have the bulk of this reconciled, and will push to this branch if all the testing works.

[WardF]

Rather than muddy the waters with more potential problems, I've put my attempted reconciliation of this PR, alongside #3094, into PR #3149. Fingers crossed, this will end with at most still needing to fix the cygwin tests, but we'll see. Assuming that everything passes (including the new CI you wired in @mannreis thank you very much!) I will be able to merge #3149, making #3122 (this PR) and #3094 redundant.