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

Support nativeTarget to build Scala Native static/shared libraries #2196

Merged
merged 11 commits into from
Oct 11, 2023

Conversation

keynmol
Copy link
Contributor

@keynmol keynmol commented Jun 9, 2023

Scala Native CLI supports building dynamic and shared libraries using --build-target argument.

So far it hasn't been supported. This PR adds the support, by

  • Adding nativeTarget directive and native-target CLI option
  • Splitting PackageType.Native into 3 different types
  • Modifying the logic to not throw when main class is not found if we are building a library
  • IMPORTANT: use -Wl,install_name argument to rename the library inside its own metadata, otherwise it points to .scala-build/project_asdkljadljk190/main - instead of test.dylib that we want

Copy link
Contributor

@lwronski lwronski left a comment

Choose a reason for hiding this comment

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

It looks promising to me, and I would be happy to merge it. Unfortunately, there is one new test that is failing. You can find more details here.

To fix the checks and reference-doc jobs, run the following commands before pushing your changes:

./mill -i __.fix
./mill -i generate-reference-doc.run

@keynmol
Copy link
Contributor Author

keynmol commented Jun 12, 2023

@lwronski, thanks, that's exactly the commands I was looking for :)

If you are happy with the overall approach I will start fixing and cleaning up the PR

@lwronski
Copy link
Contributor

@lwronski, thanks, that's exactly the commands I was looking for :)

If you are happy with the overall approach I will start fixing and cleaning up the PR

Yes, I'm satisfied with the overall approach, so I will wait for the final results.

@Gedochao
Copy link
Contributor

Gedochao commented Oct 5, 2023

@keynmol what is the current status on this? can we unblock you somehow?

@keynmol
Copy link
Contributor Author

keynmol commented Oct 5, 2023

Hey @Gedochao! I didn't forget about this PR, just was short on time.

I resolved the conflicts (but incorrectly it seems, I will address those failures).

Now that I re-ran the tests on Linux, I remember the main problem - this PR relies on passing -install_name linker flag, to make sure that the dynamic library loading entries refer to the correct (relative) location of the built dynamic library, instead of the location where it's actually built (.scala-build/.../).

Problem is - on Linux this flag doesn't seem to work

Just as I was writing those last two paragraphs I found the linux equivalent option 🤦

I'll pick this up again and hopefully will address remaining test failures.

Copy link
Contributor

@MaciejG604 MaciejG604 left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: Maciej Gajek <61919032+MaciejG604@users.noreply.github.com>
@Gedochao Gedochao merged commit 8263c39 into VirtusLab:main Oct 11, 2023
38 checks passed
@keynmol
Copy link
Contributor Author

keynmol commented Oct 11, 2023

🎉

Thanks @Gedochao and @MaciejG604

@keynmol keynmol deleted the scala-native-build-target branch October 11, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants