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

Make assets PackageCompiler-friendly #426

Merged

Conversation

MichaelHatherly
Copy link
Contributor

As discussed briefly on slack yesterday. A possible solution to relocatablilty issues with the hardcoded paths.

Embeds the asset folder contents into the precompiled files. When the original folder isn't available (due to relocation) creates a scratchspace containing the original folder contents and uses that instead. Might be worth breaking this out into a small package if we actually go with this approach.

The other option is to just go with full artifacts for now until JuliaLang/julia#38696 gives a more lightweight solution. It's a bit of a hassle to update them when things change though, hence why I've given this a try to see how it feels.

A possible solution to relocatablilty issues with the hardcoded paths.

Embeds the asset folder contents into the precompiled files. When the
original folder isn't available (due to relocation) creates a
scratchspace containing the original folder contents as uses that
instead.
@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2021

Codecov Report

Merging #426 (1dfb8d7) into master (48c99c7) will decrease coverage by 0.03%.
The diff coverage is 71.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #426      +/-   ##
==========================================
- Coverage   74.03%   74.00%   -0.04%     
==========================================
  Files          26       27       +1     
  Lines        1352     1385      +33     
==========================================
+ Hits         1001     1025      +24     
- Misses        351      360       +9     
Flag Coverage Δ
unittests 74.00% <71.87%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Weave.jl 52.17% <ø> (ø)
src/RelocatableFolders.jl 71.87% <71.87%> (ø)
src/run.jl 84.29% <0.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48c99c7...1dfb8d7. Read the comment docs.

@pfitzseb
Copy link
Member

LGTM, but I do feel like this should go into a separate package.

@MichaelHatherly
Copy link
Contributor Author

JuliaPackaging org might be a decent home for it then, do you have access to that one @pfitzseb? (In the meantime I'll push to my personal account.)

@pfitzseb
Copy link
Member

I don't. Maybe we can get @staticfloat to handle the transfer if you give him access?

@staticfloat
Copy link
Contributor

Yeah, I can do that.

@MichaelHatherly
Copy link
Contributor Author

Package is at https://github.com/MichaelHatherly/RelocatableFolders.jl and registration PR is JuliaRegistries/General#38805. I'll give @staticfloat access to that repo, and we'll just need to update the registration PR to point to the right place as well.

@staticfloat
Copy link
Contributor

I accepted the invitation, but I can't see the Settings tab, so I don't think I have high enough privileges.

@MichaelHatherly
Copy link
Contributor Author

Looks like there can only be a single "owner" of a personal repo. I've tried instead to transfer ownership to JuliaPacking but that yielded a "You don’t have the permission to create public repositories on JuliaPackaging" error.

@staticfloat
Copy link
Contributor

Try transferring it to me, and I'll transfer it to JuliaPackaging

@staticfloat
Copy link
Contributor

done

@MichaelHatherly
Copy link
Contributor Author

Bump.

@pfitzseb pfitzseb merged commit 696db1d into JunoLab:master Jun 21, 2021
@MichaelHatherly MichaelHatherly deleted the mh/package-compiler-friendly-assets branch June 21, 2021 08:00
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.

4 participants