-
-
Notifications
You must be signed in to change notification settings - Fork 12.2k
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
clang-build-analyzer 1.4.0 (new formula) #121315
Conversation
|
af4b6f5
to
11ef546
Compare
‘-ftime-trace’ is there only for clang. I made the recipe macOS only. |
Now it passes all the tests. |
Formula/clang-build-analyzer.rb
Outdated
def install | ||
system "cmake", "-S", ".", "-B", "build", *std_cmake_args | ||
system "make", "-C", "build" | ||
bin.install ["build/ClangBuildAnalyzer"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why aren't we using an install
target?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
63b33fc
to
4c546fe
Compare
Thank you for your review and changes. There was no particular reason for doing things as they were done, just inexperience with brew Formulas. I have applied all the suggested changes and squashed everything again in one single commit. |
All the tests green again. |
Formula/clang-build-analyzer.rb
Outdated
homepage "https://github.com/aras-p/ClangBuildAnalyzer" | ||
url "https://github.com/aras-p/ClangBuildAnalyzer/archive/v1.4.0.tar.gz" | ||
sha256 "dae8e7838145a72c01c397c3998d9f6801fc4dc819d552010d702cab7dede530" | ||
license "Unlicense" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not seem accurate since the source includes third-party sources with different licenses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should I do in that case? Have all the licenses as a list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. You can use the all_of:
syntax to specify an array of licenses.
dde1a46
to
c740ed1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Thank you for the detailed review. |
@ktf, 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!!!! |
brew install --build-from-source <formula>
, where<formula>
is the name of the formula you're submitting?brew test <formula>
, where<formula>
is the name of the formula you're submitting?brew audit --strict <formula>
(after doingbrew install --build-from-source <formula>
)? If this is a new formula, does it passbrew audit --new <formula>
?