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

Add resources support to Scala Native and fixes to overall resources #812

Merged
merged 3 commits into from
Sep 12, 2022
Merged

Add resources support to Scala Native and fixes to overall resources #812

merged 3 commits into from
Sep 12, 2022

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented Mar 25, 2022 •
edited
Loading

This PR allows to embed resources (by default) in a SN binary, as well as fixes some bugs that ended up being related. Those can be then accessed via getResourceAsStream Java API. An integration test was also added.

While working on this I ended up fixing a couple bugs that did not have a chance to show until now, connected with scala-cli SN support:

  • SN caching: previously only resources passed with a cli-option would be cached, using directive ones would be ignored.
  • classpath passed to SN: previously I think I unnecessarily doubled it for non-script projects, which ended up messing with the resources

I later got some feedback and decided to also allow using c files as regular inputs, instead of using them via resources (which is basically how the underlying Scala Native handles them). This required me to make a very simple fix of #808 (and #1209), which I did for both the newly added C files, and regular java-style resources.
We now store in .scala-build an additional file which stores mapping between the path of the original c file/resource and the one copied internally for building. This way we can delete the internal files in case it disappears in the original path, which eliminates files doubling when renamed etc.

In summary:

  • embedding resources was added by default for SN resources
  • a new option "-no-embed", was added for SN which will not embed any resources (useful for keeping overall binary size low in the more complex builds)
  • various improvements/ fixes were done with SN builds
  • passing c files as regular inputs was added, which are then handled by SN allowing for easier interop in scala-cli
  • Both c files and regular resources have internal .scala-build states consistent with the project by the use of ResourceMapper, which maps source file (in project) to destination (in .scala-build), before copying
  • fixes Deleted resources are not being removed from .scala-build and the classpath #808 and Not recognizing removed resources #1209

@jchyb jchyb changed the title Add resource support to Scala Native Add resources support to Scala Native Mar 25, 2022
@jchyb jchyb force-pushed the native-resources branch 3 times, most recently from 78f2480 to dd0d7f0 Compare July 18, 2022 18:55
@jchyb jchyb force-pushed the native-resources branch 2 times, most recently from 9d7f503 to 9cfe915 Compare August 17, 2022 07:42
@jchyb jchyb changed the title Add resources support to Scala Native Add resources support to Scala Native and fixes to overall resources Aug 17, 2022
@jchyb
Copy link
Contributor Author

jchyb commented Aug 17, 2022

I have heavily updated the PR message to reflect the new scope of the PR (now with fixes for the java resources outside of SN)

@jchyb jchyb marked this pull request as ready for review August 17, 2022 07:45
alexarchambault
alexarchambault reviewed Aug 17, 2022
View reviewed changes
Copy link
Contributor

@alexarchambault alexarchambault left a comment

Choose a reason for hiding this comment

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

I see you just pushed things. Here's my in progress review comments.

modules/build/src/main/scala/scala/build/ResourceMapper.scala Outdated Show resolved Hide resolved
modules/build/src/main/scala/scala/build/ResourceMapper.scala Outdated Show resolved Hide resolved
modules/cli-options/src/main/scala/scala/cli/commands/ScalaNativeOptions.scala Outdated Show resolved Hide resolved
modules/cli-options/src/main/scala/scala/cli/commands/ScalaNativeOptions.scala Outdated Show resolved Hide resolved
modules/cli-options/src/main/scala/scala/cli/commands/ScalaNativeOptions.scala Outdated Show resolved Hide resolved
modules/cli/src/main/scala/scala/cli/commands/Package.scala Outdated Show resolved Hide resolved
modules/build/src/main/scala/scala/build/Inputs.scala Outdated Show resolved Hide resolved
modules/build/src/main/scala/scala/build/Inputs.scala Outdated Show resolved Hide resolved
modules/build/src/main/scala/scala/build/ResourceMapper.scala Outdated Show resolved Hide resolved
@jchyb
Copy link
Contributor Author

jchyb commented Aug 17, 2022

Sorry about sudden pushing, I did it to (hopefully) fix the issues with the failing gif tests in the CI, the committed code was not changed, only rebased

alexarchambault
alexarchambault reviewed Aug 17, 2022
View reviewed changes
Copy link
Contributor

@alexarchambault alexarchambault left a comment •
edited
Loading

Choose a reason for hiding this comment

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

About the resource mapping stuff: do we really need to write an actual mapping file on disk like this? Couldn't we instead write a list of relative paths of the resources written in the output dir, and right before copying / updating resources, look at those that will not be overwritten, and remove them?

The resource safety thing (ensuring we remove stale resources) is really great, but maybe it could be done in a separate PR?

modules/build/src/main/scala/scala/build/ResourceMapper.scala Outdated Show resolved Hide resolved
modules/build/src/main/scala/scala/build/ResourceMapper.scala Outdated Show resolved Hide resolved
@armanbilge armanbilge mentioned this pull request Aug 25, 2022
Closed
@jchyb jchyb force-pushed the native-resources branch 2 times, most recently from e0a89c5 to 4c7e369 Compare August 25, 2022 14:12
@jchyb
Copy link
Contributor Author

jchyb commented Aug 25, 2022

I have updated the mapping file contents to store only the relative output paths, as was suggested. I am not sure about the gains splitting the PR would give us (outside of easier review), since both the SN improvements and fixes to overall fixes to resources depend on the mapping mechanizm - I imagine if something goes wrong we would still have to revert both.

@alexarchambault alexarchambault force-pushed the native-resources branch 2 times, most recently from 337caf5 to 770a0d3 Compare August 27, 2022 01:07
@alexarchambault
Copy link
Contributor

I reworked a bit the commit history, to make it easier to review this. I'll have a closer look a bit later.

@alexarchambault

This comment was marked as outdated.

Sign in to view
@alexarchambault
Copy link
Contributor

alexarchambault commented Sep 5, 2022 •
edited
Loading

So I rebased on the latest main, and refactored things a bit further. AFAIC, I'm happy with the result (hoping the CI will be too).

@alexarchambault

This comment was marked as outdated.

Sign in to view
@jchyb @lwronski
Fix deleting/renaming resources
bdd782b 
Previously this would have no effect on the program. With this commit a
mapping feature was introduced to resources, which remembers from where
a resource file originates.

An integration test for the issue was also added.
@jchyb @lwronski
Add embedding resources support for Scala Native
7145b1f 
Works automatically when a resourceDir option or using Directory is
added. Having that working effectively required fixing duplicated
classpath in Scala Native packaging. Will work only on Scala Native
0.4.4 and up. Caching was also improved, as previously only the cli
option resources would be cached, and using directive resources would
be ignored.

A --embed-resources option was also added, allowing to disable resource
embedding (which will also make the resources API not work). The use
case for this is using c interop without having unnecessary libraries
metadata embedded, taking up space in a binary file.
@jchyb @lwronski
Allow to pass Scala Native c interop file as an Input
6f25287 
This should provide simpler interop functionality, removing to rely on
passing c files through resource directories.
A new resource caching mechanizm was also added, to smoothen the
development experience. Before, it would be easy for SN linking errors
to appear - any rename or move of an interop file would cause it to be
duplicated. The mappingremoves those issues, as long as users will not
tamper with the newly added file (.project_native_resources) to output
directory. This can be extended for use with regular JVM resources, to
solve a similar issue.
@lwronski lwronski mentioned this pull request Sep 12, 2022
Open
@lwronski lwronski merged commit 5f15ada into VirtusLab:main Sep 12, 2022
@lwronski lwronski mentioned this pull request Sep 14, 2022
Closed
@Gedochao Gedochao added the enhancement New feature or request label Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lwronski lwronski lwronski left review comments

@romanowski romanowski romanowski approved these changes

@alexarchambault alexarchambault alexarchambault approved these changes

@Gedochao Gedochao Gedochao approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deleted resources are not being removed from .scala-build and the classpath
5 participants
@jchyb @alexarchambault @romanowski @Gedochao @lwronski