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.

Sign up for GitHub

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

Jump to bottom

Attempt to use ar command for static libraries when possible #3548

Merged
merged 3 commits into from Oct 9, 2023
Merged

Attempt to use ar command for static libraries when possible #3548

merged 3 commits into from Oct 9, 2023

Conversation

keynmol
Copy link
Contributor

@keynmol keynmol commented Oct 7, 2023

Closes #3542

@keynmol keynmol marked this pull request as ready for review October 7, 2023 06:49
WojciechMazur
WojciechMazur approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@WojciechMazur WojciechMazur left a comment

Choose a reason for hiding this comment

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

Thank you for this change, looks good to me. Could please only rebase this branch on the current HEAD just to be sure? At the time it was submitted the mac-os CI was broken, sorry for that.

@keynmol keynmol force-pushed the use-ar-for-static-libraries branch from 0b2fc11 to 2f9ba15 Compare October 9, 2023 12:35
@keynmol keynmol mentioned this pull request Oct 9, 2023
Merged
@keynmol
Copy link
Contributor Author

keynmol commented Oct 9, 2023 •
edited

@WojciechMazur seems like my dynamic lib PR was merged with a failure: https://github.com/scala-native/scala-native/actions/runs/6439525000/job/17488991160#step:4:342

I will investigate and provide a fix in this PR.

EDIT: I believe this is the fix: cfae0d9, at least it fixed the test locally

@keynmol
Copy link
Contributor Author

keynmol commented Oct 9, 2023

If you need help backporting this into 0.4.x - please let me know, I really want to get this into the next release, just to avoid setting up LLVM on Scala-cli :D VirtusLab/scala-cli#2196 (comment)

@WojciechMazur
Copy link
Contributor

Sorry about merged that one too early. Only this PR seemed to be able to break something. Should have waited for the rebase and not skip the procedure

@WojciechMazur
Copy link
Contributor

If you need help backporting this into 0.4.x - please let me know

I think it should apply quite smoothly, but I won't hesistate to ask for clues if needed. Thank you. I'd like a make a next 0.4.x patch by the end of this week

@LeeTibbert LeeTibbert mentioned this pull request Oct 9, 2023
Open
@WojciechMazur WojciechMazur merged commit 9b1b616 into scala-native:main Oct 9, 2023
81 checks passed
@keynmol keynmol deleted the use-ar-for-static-libraries branch October 9, 2023 17:02
WojciechMazur pushed a commit to WojciechMazur/scala-native that referenced this pull request Oct 13, 2023
@keynmol @WojciechMazur
Attempt to use ar command for static libraries when possible (scala…
e11eb95 
…-native#3548)

* Attempt to use `ar` command for static libraries when possible
* Fix ar invocation withour MRI script
* Correctly pass linker flags

(cherry picked from commit 9b1b616)
WojciechMazur pushed a commit that referenced this pull request Oct 13, 2023
@keynmol @WojciechMazur
Attempt to use ar command for static libraries when possible (#3548)
25e4b0a 
* Attempt to use `ar` command for static libraries when possible
* Fix ar invocation withour MRI script
* Correctly pass linker flags

(cherry picked from commit 9b1b616)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WojciechMazur WojciechMazur WojciechMazur approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider defaulting to ar command for static linking, instead of llvm-ar
2 participants
@keynmol @WojciechMazur