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

itk 4.13.2 (new formula) #39138

Closed
wants to merge 1 commit into from
Closed

itk 4.13.2 (new formula) #39138

wants to merge 1 commit into from

Conversation

fblupi
Copy link
Contributor

@fblupi fblupi commented Apr 21, 2019

  • 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?
  • Is your test running fine brew test <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>)?

Moving formula from homebrew-sci as they say they don't keep maintaining the repo. I have renamed the formula from insighttoolkit to itk and updated it to a more recent version.

@BrewTestBot
Copy link
Member

  • New formulae should not have a bottle do block

@fxcoudert fxcoudert added the new formula PR adds a new formula to Homebrew/homebrew-core label Apr 21, 2019
@fxcoudert
Copy link
Member

  * Non-libraries were installed to "/usr/local/opt/itk/lib"
    Installing non-libraries to "lib" is discouraged.
    The offending files are:
      /usr/local/opt/itk/lib/itkLIBMINCConfig.cmake
            /usr/local/opt/itk/lib/UseitkLIBMINC.cmake

@fblupi
Copy link
Contributor Author

fblupi commented Apr 21, 2019

  • New formulae should not have a bottle do block

Fixed

  • Non-libraries were installed to "/usr/local/opt/itk/lib"
    Installing non-libraries to "lib" is discouraged.
    The offending files are:
    /usr/local/opt/itk/lib/itkLIBMINCConfig.cmake
    /usr/local/opt/itk/lib/UseitkLIBMINC.cmake

I think I can't do anything to solve this. I can't detect what cmake option is generating the cmake file in the lib folder and the formula from the homebrew-sci repo generate this cmake file too.

@fxcoudert
Copy link
Member

the formula from the homebrew-sci repo generate this cmake file too

That does not make it suitable for homebrew-core.

 Migrated from Homebrew/science
@zbeekman
Copy link
Contributor

libminc (vendored by itk) needs updating it's CMakeLists.txt. I've added a patch, and submitted a PR to ITK, which is being investigated upstream of that by MINC too.

@fblupi do you mind if I force push to your branch with my changes (and rebased onto a more recent homebrew-core/master)?

@zbeekman
Copy link
Contributor

@fblupi I hope you don't mind: I'm going to force push to your branch so that this can get tested with CI.

@fblupi
Copy link
Contributor Author

fblupi commented Apr 23, 2019

@zbeekman No problem. Feel free :)

@BrewTestBot
Copy link
Member

  • Formulae should not require patches to build. Patches should be submitted and accepted upstream first.

@zbeekman
Copy link
Contributor

Yes, we know brew testbot...

@zbeekman
Copy link
Contributor

@fxcoudert what are your thoughts on merging it? It looks like the upstream of the upstream is patching this, and the upstream package is going to incorporate that patch.

@zbeekman
Copy link
Contributor

Thanks for your contribution to Homebrew! 🎉 🥇

Without awesome contributors like you, it would be impossible to maintain Homebrew to the high level of quality users have come to expect. Thank you!!!!

@zbeekman zbeekman closed this in c5a0ddd Apr 24, 2019
@fblupi
Copy link
Contributor Author

fblupi commented Apr 25, 2019

Thank you too @zbeekman for your help with the issue I had! 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants