scala-cli icon indicating copy to clipboard operation
scala-cli copied to clipboard

Add resources support to Scala Native and fixes to overall resources

Open jchyb opened this issue 3 years ago • 4 comments
trafficstars

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 #808 and #1209

jchyb avatar Mar 25 '22 08:03 jchyb

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 avatar Aug 17 '22 07:08 jchyb

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

jchyb avatar Aug 17 '22 13:08 jchyb

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.

jchyb avatar Aug 25 '22 14:08 jchyb

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

alexarchambault avatar Aug 27 '22 01:08 alexarchambault

I'm in the midst of reviewing this. I might push a slightly reworked version of it, either here or in another PR.

alexarchambault avatar Sep 05 '22 16:09 alexarchambault

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 avatar Sep 05 '22 18:09 alexarchambault

Rebased again, hoping it will fix CI.

alexarchambault avatar Sep 06 '22 09:09 alexarchambault