Skip to content
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

Improve requirements #178

Merged
merged 23 commits into from
Jul 5, 2023
Merged

Improve requirements #178

merged 23 commits into from
Jul 5, 2023

Conversation

zklaus
Copy link

@zklaus zklaus commented Jul 3, 2023

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

The main goal of this PR is to sort out any overlinking-related warnings. The main tool for that is fixing any dependency irks. In the process, this is what came out:

  • Enable zip support for zarr to justify dependency on libzip
  • Fix Bzip2 plugin installation by a patch to the Cmake build system
  • Remove obsolete msinttypes dependency that was only necessary for old vc versions
  • Replace curl dependency with libcurl dependency, which also brings the right run_export
  • Add libaec dependency which provides libsz, but was missing so far
  • Order deps alphabetically
  • Simplify run reqs with better use of run_exports.

It also aligns the Windows and Linux builds more closely by

  • Using the same ordering of options in the Cmake line if they are the same
  • Adding the following to Windows, which was only in the Linux build up to now
    • BUILD_UTILITES=ON
    • ENABLE_DOXYGEN=OFF
    • ENABLE_DAP=ON
    • ENABLE_NETCDF_4=ON
    • ENABLE_PLUGIN_INSTALL=ON
  • Adding the following to Linux which was only in the Windows build up to now
    • ENABLE_BYTERANGE=ON

Klaus Zimmermann added 2 commits July 3, 2023 14:20
- depend on libcurl instead of curl to get right run_exports
- add libaec dependency
@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@zklaus
Copy link
Author

zklaus commented Jul 3, 2023

@conda-forge-admin, please rerender

conda-forge-webservices[bot] and others added 3 commits July 3, 2023 12:26
recipe/meta.yaml Outdated Show resolved Hide resolved
@zklaus
Copy link
Author

zklaus commented Jul 3, 2023

@conda-forge-admin, please rerender

@github-actions
Copy link
Contributor

github-actions bot commented Jul 3, 2023

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/libnetcdf-feedstock/actions/runs/5444595782.

@zklaus
Copy link
Author

zklaus commented Jul 3, 2023

@conda-forge-admin, please rerender

@github-actions
Copy link
Contributor

github-actions bot commented Jul 3, 2023

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/libnetcdf-feedstock/actions/runs/5446069775.

@zklaus
Copy link
Author

zklaus commented Jul 5, 2023

Linux

ThreeOne linker warnings remains:

WARNING (libnetcdf): dso library package conda-forge::libstdcxx-ng-13.1.0-hfd8a6a1_0 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)
  • I fixed the Bzip2 part related to Bzip2 plugin is not installed #179 in a patch that I am upstreaming at Switch custom Bzip2 cmake module to standard Unidata/netcdf-c#2718.
  • Libjpeg-turbo comes in only as a dependency of HDF4 and is now removed from requirements.
  • The C++ standard library is a bit strange. My best guess now is that C++ is used only for a few small files that don't introduce any actual dependency on the standard library like a normal program or even a fully-fledged library would. I suggest to leave this issue for another time.

Osx

Like Linux, only different C++ runtime

WARNING (libnetcdf): dso library package conda-forge::libcxx-16.0.6-hd57cbcb_0 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)

Windows

Slightly worse with

WARNING (libnetcdf): run-exports library package conda-forge::libaec-1.0.6-h63175ca_1 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)
WARNING (libnetcdf): run-exports library package conda-forge::libzlib-1.2.13-hcfcfb64_5 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)
WARNING (libnetcdf): run-exports library package conda-forge::blosc-1.21.4-hdccc3a2_0 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)
WARNING (libnetcdf): run-exports library package conda-forge::bzip2-1.0.8-h8ffe710_4 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)
WARNING (libnetcdf): run-exports library package conda-forge::zstd-1.5.2-h12be248_6 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)

I think the additional warnings come about because the checker doesn't know to include the hdf5 plugin dir in its consideration because it is not in the same library directory as the rest, in other words, I think these warnings are spurious.

@zklaus

This comment was marked as outdated.

@zklaus zklaus marked this pull request as ready for review July 5, 2023 14:23
@zklaus
Copy link
Author

zklaus commented Jul 5, 2023

Beware of issues flagged in #160 and #166!

Copy link
Contributor

@xylar xylar left a comment

Choose a reason for hiding this comment

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

I would certainly like someone with more expertise than me to have a look but this looks really excellent to me.

Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Seems generally reasonable to me.

@ocefpaf
Copy link
Member

ocefpaf commented Jul 5, 2023

Ok, 3 approvals is good enough :-)
Let's get this one merged.

Thanks @zklaus for the awesome work you put into this!

@ocefpaf ocefpaf merged commit 3435c60 into conda-forge:main Jul 5, 2023
@zklaus zklaus mentioned this pull request Jul 6, 2023
5 tasks
@zklaus zklaus deleted the improve-reqs branch July 6, 2023 10:21
@zklaus
Copy link
Author

zklaus commented Jul 6, 2023

Cheers guys 😃

This was referenced Jul 6, 2023
@valeriupredoi
Copy link

@zklaus is this now compiled with byte-range flag on? (sorry, didn't want to go through all the changed files haha). If so then this means we now have S3 support by my back-of-the-enevolope calculations? Many thanks, bud 🍺

@zklaus
Copy link
Author

zklaus commented Jul 6, 2023

Yes, @valeriupredoi, byte-range is available now. And I think you are right that this is enough for S3 access, particularly for single netcdf4 files. Though as I understand the discussion in #160 and #166, you may need to tell libnetcdf about it by appending something like #mode=bytes to the URL.

@valeriupredoi
Copy link

valeriupredoi commented Jul 6, 2023

Ooh nice thanks bud - am gonna reference your comment here to our issue, so I don't forget both the info and the references, it's getting a bit Google Map-y with all these issues and PRs. Cheers muchly 🍺

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.

5 participants