Skip to content

Patch #3090 - Fix and test S3 enabled builds #3122

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

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mannreis
Copy link
Contributor

@mannreis mannreis commented Apr 15, 2025

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 #3147

@DennisHeimbigner
Copy link
Collaborator

-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
Copy link
Contributor Author

mannreis commented Jun 12, 2025

@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 mannreis changed the title Patch #3090 Patch #3090 - Fix and test S3 enabled builds Jun 25, 2025
@mannreis mannreis marked this pull request as ready for review June 25, 2025 15:07
@mannreis
Copy link
Contributor Author

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
Copy link
Collaborator

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

@mannreis mannreis force-pushed the patch-3090 branch 5 times, most recently from 6f8324e to 5d4a2c7 Compare June 26, 2025 12:34
@mannreis mannreis marked this pull request as draft June 26, 2025 12:44
@mannreis mannreis force-pushed the patch-3090 branch 3 times, most recently from 572c200 to 3dbbbf8 Compare June 26, 2025 13:32
@DennisHeimbigner
Copy link
Collaborator

I am seriously confused about what is going on here.

@WardF
Copy link
Member

WardF commented Jun 26, 2025

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

@mannreis
Copy link
Contributor Author

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
Copy link
Member

WardF commented Jul 1, 2025

@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
Copy link
Contributor Author

mannreis commented Jul 3, 2025

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
Copy link
Collaborator

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
Copy link
Member

WardF commented Jul 7, 2025

@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
Copy link
Member

WardF commented Jul 7, 2025

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
Copy link
Member

WardF commented Jul 9, 2025

@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
Copy link
Contributor Author

mannreis commented Jul 9, 2025

@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
Copy link
Member

WardF commented Jul 9, 2025

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

@WardF
Copy link
Member

WardF commented Jul 9, 2025

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

WardF added a commit to WardF/netcdf-c that referenced this pull request Jul 9, 2025
WardF added a commit to WardF/netcdf-c that referenced this pull request Jul 9, 2025
@WardF
Copy link
Member

WardF commented Jul 9, 2025

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.

mannreis added a commit to mannreis/netcdf-c that referenced this pull request Jul 11, 2025
…autotools behavior (in support of Unidata#3122"

This reverts commit 42dbe33.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failing builds with S3 enabled
3 participants