-
-
Notifications
You must be signed in to change notification settings - Fork 12.3k
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
odin: added odin libraries for linux #182310
odin: added odin libraries for linux #182310
Conversation
Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request. |
The solution is not to duplicate the block, but to figure out which parts should only be on macOS and make the rest unconditional. It also needs test adjustments so we know it works. |
@SMillerDev Do I need to update the test? |
Yes, because without it there is no way to know if this works |
2848068
to
ad692a8
Compare
588ab6a
to
e68fa14
Compare
2f525c7
to
b912b6a
Compare
b912b6a
to
b24d467
Compare
@SMillerDev I have made the changes and as per @laytan the issue with the shared library is an upstream issue with odin, is there any more change required to the code? |
b24d467
to
6b05bb4
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.
Thanks for your work here; this is looking good. Let's just DRY it up.
6b05bb4
to
a4821e5
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.
Thanks, looking good! A few minor tweaks.
After applying these, please run
brew style --fix odin
to fix any remaining brew
linter issues.
b74d29a
to
14dfa31
Compare
@carlocab I've made all the requested changes and I am pushing my new code in a bit kindly review that for any changes required. |
Linked back the vendor libraries for odin on linux. Use of shared library is still not working as it is an upstream issue. Closes Homebrew#182068 odin: added vendor deps and build from source for odin odin: changes .dylib to .so odin: fixed lint odin: fixed lint odin: attempted fixes for macos-arm64 odin: attempted fixes for macos-arm64 odin: attempted fixes for macos-arm64 odin: attempted fixes for macos-arm64 odin: added tests to verify it runs on linux odin: fixed tests for linux odin: fixed tests for linux Fixed all tests for linux. Closes Homebrew#182068 odin: fixed tests Fixed tests for linux. Closes Homebrew#182068 odin: fixes shared library on linux Closes Homebrew#182068 The tags during the building and linking phase where placed in the wrong way and the gcc compiler was not treating the linking of the shared objects as a shared library. Fixed them and all tests run perfectly. odin: changed the structure to if...else... block changed the structure of code as per review. odin: fixed all tests Closes Homebrew#182068 Fixed all tests. The shared library test fails because of issue with odin. odin: changed the code to make build steps unconditional odin: fixed styling issues. odin: made the requested changes. odin: fixed linting issues.
14dfa31
to
b13a6ef
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.
Thank you; very nice work!
🤖 An automated task has requested bottles to be published to this PR. |
HOMEBREW_NO_INSTALL_FROM_API=1 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 doingHOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>
)? If this is a new formula, does it passbrew audit --new <formula>
?Closes #182068
I've added the vendor libraries for linux and built them and also changed the test to verify it on linux.