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

lightgbm 2.2.3 (new formula) #43244

Closed
wants to merge 1 commit into from
Closed

lightgbm 2.2.3 (new formula) #43244

wants to merge 1 commit into from

Conversation

ankane
Copy link
Contributor

@ankane ankane commented Aug 18, 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>)?

@lembacon lembacon added the new formula PR adds a new formula to Homebrew/homebrew-core label Aug 18, 2019
@SMillerDev
Copy link
Member

Please don't specify the amount of jobs in the makefile. It might cause issues with building.

@ankane
Copy link
Contributor Author

ankane commented Aug 18, 2019

My bad, updated

system "cmake", *args, ".."
system "make"
end
lib.install "lib_lightgbm.so"
Copy link
Member

Choose a reason for hiding this comment

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

Shared libraries on macOS should be .dylib, not .so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this as well with an additional flag.

system "make"
end
lib.install "lib_lightgbm.so"
bin.install "lightgbm"
Copy link
Member

Choose a reason for hiding this comment

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

Is there no make install target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @fxcoudert, thanks for the feedback. There was a make install target, so switched to that.

@fxcoudert
Copy link
Member

This is great! Merging.

@fxcoudert fxcoudert closed this in 92fda6f Sep 14, 2019
@ankane
Copy link
Contributor Author

ankane commented Sep 14, 2019

Thanks @fxcoudert and @SMillerDev 🎉

@ankane ankane deleted the lightgbm branch September 14, 2019 19:30
@ankane ankane restored the lightgbm branch September 14, 2019 19:31
@lock lock bot added the outdated PR was locked due to age label Jan 9, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants