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

[netcdf-c] Fix dependency libmath #12434

Merged
merged 6 commits into from
Aug 10, 2020
Merged

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Jul 15, 2020

Since netcdf-c requires the math library, the system in Linux also provides libm, but embree3 also generates the libmath library. Due to search order issues, embree's libmath library will be used first.

So change the math library name order to prevent the use of embree's math library.

Fixes #12169

@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Jul 15, 2020
@Neumann-A
Copy link
Contributor

No that is incorrect. netcdf-c searches for any usable math library. This might be either libmath.a or a system provided libm.a. As such the CMakeLists.txt needs to differentiate between those two a bit more and have a option like USE_LIBMATH_INSTEAD_OF_LIBM/USE_EMBREE_LIBMATH or similar. It is not a necessity to build netcdf-c with libmath. A system provided libm would also be fine.

@JackBoosY
Copy link
Contributor Author

@Neumann-A Yes, the math library can be provided by the system, but not necessarily.
We cannot determine whether the math library exists in the build environment, so we must rely on embree to ensure a smooth build.

@Neumann-A
Copy link
Contributor

@JackBoosY: A lot of ports would fail in CI if -lm wouldn't be available.

If you change the order to
FIND_LIBRARY(HAVE_LIBM NAMES m libm math)
or simply remove math than you should have what is intended in netcdf-c. I think that it picked up libmath.a from embree was more by accident than by design

@Neumann-A
Copy link
Contributor

considering that the commit introducing the find_library call does not even have a commit message and was 8 years ago.

@JackBoosY JackBoosY changed the title [netcdf-c] Add dependency embree3 [netcdf-c] Fix dependency libmath Jul 16, 2020
@JackBoosY
Copy link
Contributor Author

@Neumann-A Should be okay now.

@JackBoosY JackBoosY marked this pull request as ready for review August 6, 2020 02:32
ports/netcdf-c/CONTROL Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@PhoebeHui PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label Aug 10, 2020
@strega-nil strega-nil merged commit 7ad0f08 into microsoft:master Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[netCDF] Fix dependency on embree (ParaView CI error)
5 participants