Skip to content

CMake: Bring nczarr dependency when enabling S3 in line with autotools - #3149

Merged
WardF merged 29 commits into
Unidata:mainfrom
WardF:s3-nczarr-cmake-ac-fix.wif
Aug 1, 2025
Merged

CMake: Bring nczarr dependency when enabling S3 in line with autotools#3149
WardF merged 29 commits into
Unidata:mainfrom
WardF:s3-nczarr-cmake-ac-fix.wif

Conversation

@WardF

@WardF WardF commented Jul 9, 2025

Copy link
Copy Markdown
Member

In support of #3122

DennisHeimbigner and others added 13 commits February 16, 2025 17:53
re: PR Unidata#3068

Affects files:
* libdispatch/ncutil.[ch]
* libdispatch/drc.c
* include/ncutil.h
* include/ncrc.h
* include/netcdf_vutils.h
plus any files that use the above modified .h files.

Part of splitting PR 3068

1. move libdispatch/ncutil.h to include/netcdf_vutils.h and rename uses.
2. Refactor code to move and unify various functions from libdispatch/drc.c into ncutil.h and ncutil.c.
3. Modify file includes to use ncutil.h.
@WardF
WardF requested a review from DennisHeimbigner as a code owner July 9, 2025 18:02

@mannreis mannreis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I left some minor suggestions to remove duplicate imports and a comment on the build that is failing. Thanks!

Comment thread CMakeLists.txt
Comment on lines +775 to +780
# If NETCDF_ENABLE_S3 is ON/TRUE, then enable NETCDF_ENABLE_NCZARR, in order to keep
# things congruent with autotools.
if (NETCDF_ENABLE_S3)
set(NETCDF_ENABLE_NCZARR ON CACHE BOOL "Enable NCZarr Client" FORCE)
endif(NETCDF_ENABLE_S3)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this is causing the build problem in: https://github.com/Unidata/netcdf-c/actions/runs/16181140174/job/45678589358?pr=3149#step:19:1518

By setting S3 on, NCZARR is turned on but it will still need HDF5/NC4 therefore failing the build with NC3.

I think the issue was with autotools only on CYGWIN, not in cmake. Going back to my pipeline: https://github.com/Unidata/netcdf-c/actions/runs/15902006750/job/44847012519#step:13:686
The libcrypto symbols were not being found when --enable-s3 and --disable-nczarr combined.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@WardF, to address this comment I've added a few modifications to configure.ac that you bring in via WardF#2

Comment thread libdispatch/dinfermodel.c Outdated
Comment thread libdispatch/ds3util.c Outdated
Comment thread nc_test/tst_inmemory.c Outdated
Comment thread oc2/xxdr.c Outdated
WardF and others added 6 commits July 14, 2025 16:18
Co-authored-by: Manuel Reis <16836000+mannreis@users.noreply.github.com>
Co-authored-by: Manuel Reis <16836000+mannreis@users.noreply.github.com>
Co-authored-by: Manuel Reis <16836000+mannreis@users.noreply.github.com>
Co-authored-by: Manuel Reis <16836000+mannreis@users.noreply.github.com>
@WardF

WardF commented Jul 27, 2025

Copy link
Copy Markdown
Member Author

Just a note I am still working on this @DennisHeimbigner @mannreis. Can anybody replicate the cygwin issues locally? I'm struggling to. I'm out Monday-Wednesday, getting back midday Thursday. This blocking issue needs to be resolved before I can move on to other business, and I appreciate any thoughts. I'm sure we'll get it :)

@mannreis

mannreis commented Jul 29, 2025

Copy link
Copy Markdown
Contributor

@WardF @DennisHeimbigner the problem was that the unzip executable being used was from the git subsystem (/cygdrive/c/Program Files/Git/usr/bin/unzip) and not from cygwin (/usr/bin/unzip or /cygdrive/c/cygwin/bin/unzip).

For some reason that doesn't work... I've created WardF#3 to just add unzip and zip to be installed on cygwin setup.

You can see the output of the action from my fork: https://github.com/mannreis/netcdf-c/actions/runs/16591032738/job/46926479610. Although it fails it's because of artifact name conflict and overwrite:false

@WardF
WardF merged commit 38bec1c into Unidata:main Aug 1, 2025
102 checks passed
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.

3 participants