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: build both static and shared libraries #18854

Closed
wants to merge 2 commits into
base: master
from

Conversation

4 participants
@jeroen
Contributor

jeroen commented Oct 2, 2017

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Adds an option to produce static library of netcdf instead of a dynamic library. We really need this to build our application. No side effects on standard installation. Thank you for considering.

@@ -37,7 +39,11 @@ class Netcdf < Formula
def install
ENV.deparallelize
common_args = std_cmake_args << "-DBUILD_SHARED_LIBS=ON"

This comment has been minimized.

@DomT4

DomT4 Oct 2, 2017

Contributor
common_args = std_cmake_args

if build.with? "static"
  common_args << "-DBUILD_SHARED_LIBS=OFF"
else
  common_args << "-DBUILD_SHARED_LIBS=ON"
end
@DomT4

DomT4 Oct 2, 2017

Contributor
common_args = std_cmake_args

if build.with? "static"
  common_args << "-DBUILD_SHARED_LIBS=OFF"
else
  common_args << "-DBUILD_SHARED_LIBS=ON"
end
Show outdated Hide outdated Formula/netcdf.rb
@jeroen

This comment has been minimized.

Show comment
Hide comment
@jeroen

jeroen Oct 2, 2017

Contributor

@DomT4 Thank you. I have removed the redundant condition from the test now.

Contributor

jeroen commented Oct 2, 2017

@DomT4 Thank you. I have removed the redundant condition from the test now.

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 2, 2017

Contributor

Rather than an option. Let's just build both. See snappy.

Contributor

ilovezfs commented Oct 2, 2017

Rather than an option. Let's just build both. See snappy.

@ilovezfs

both please

@jeroen jeroen changed the title from netcdf: add option --with-static to netcdf: build both static and shared libraries Oct 3, 2017

@jeroen

This comment has been minimized.

Show comment
Hide comment
@jeroen

jeroen Oct 3, 2017

Contributor

Thanks for reviewing @ilovezfs. I updated it to build both shared and static.

For some reason we cannot just call make install twice because running the second make install will uninstall the first one. Therefore we just manually lib.install the static libs.

Contributor

jeroen commented Oct 3, 2017

Thanks for reviewing @ilovezfs. I updated it to build both shared and static.

For some reason we cannot just call make install twice because running the second make install will uninstall the first one. Therefore we just manually lib.install the static libs.

@ilovezfs

For some reason we cannot just call make install twice because running the second make install will uninstall the first one. Therefore we just manually lib.install the static libs.

This needs to be reported upstream

@jeroen

This comment has been minimized.

Show comment
Hide comment
@jeroen

jeroen Oct 7, 2017

Contributor

ok

Contributor

jeroen commented Oct 7, 2017

ok

@dunn dunn added the upstream issue label Oct 12, 2017

@jeroen

This comment has been minimized.

Show comment
Hide comment
@jeroen

jeroen Oct 12, 2017

Contributor

This works fine as is. I don't think upstream intends on changing this behavior.

Contributor

jeroen commented Oct 12, 2017

This works fine as is. I don't think upstream intends on changing this behavior.

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 12, 2017

Contributor

Did you report it?

Contributor

ilovezfs commented Oct 12, 2017

Did you report it?

@jeroen

This comment has been minimized.

Show comment
Hide comment
@jeroen
Contributor

jeroen commented Oct 12, 2017

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 12, 2017

Contributor

@jeroen yeah it would be cool if they can support doing both at once in a single make install, but, at minimum, if they are going to require us to call make install twice, the second make install shouldn't wipe out the libraries from the first make install.

Contributor

ilovezfs commented Oct 12, 2017

@jeroen yeah it would be cool if they can support doing both at once in a single make install, but, at minimum, if they are going to require us to call make install twice, the second make install shouldn't wipe out the libraries from the first make install.

@jeroen

This comment has been minimized.

Show comment
Hide comment
@jeroen

jeroen Oct 12, 2017

Contributor

Right, I will address this to upstream as well so that they can have a look at it the next time they are revising the cmake configuration.

However it may be a while until they have a chance to look at this, and then more time until the next release. Is there any chance we can move forward with this PR in the mean time? The current code seems like the obvious solution to add static libs to the build?

Contributor

jeroen commented Oct 12, 2017

Right, I will address this to upstream as well so that they can have a look at it the next time they are revising the cmake configuration.

However it may be a while until they have a chance to look at this, and then more time until the next release. Is there any chance we can move forward with this PR in the mean time? The current code seems like the obvious solution to add static libs to the build?

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 12, 2017

Contributor

@BrewTestBot test this please

Contributor

ilovezfs commented Oct 12, 2017

@BrewTestBot test this please

@jeroen

This comment has been minimized.

Show comment
Hide comment
@jeroen

jeroen Oct 12, 2017

Contributor

thanks!

Contributor

jeroen commented Oct 12, 2017

thanks!

@jeroen

This comment has been minimized.

Show comment
Hide comment
@jeroen

jeroen Oct 16, 2017

Contributor

Merge? 🚀

Contributor

jeroen commented Oct 16, 2017

Merge? 🚀

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 17, 2017

Contributor

Thanks again @jeroen!

Contributor

ilovezfs commented Oct 17, 2017

Thanks again @jeroen!

@ilovezfs ilovezfs closed this in 24f32ab Oct 17, 2017

ylluminarious added a commit to ylluminarious/homebrew-core that referenced this pull request Oct 21, 2017

netcdf: build both shared and static libraries
Closes #18854.

Signed-off-by: ilovezfs <ilovezfs@icloud.com>

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.