davidanthoff / Electron.jl

Julia wrapper for Electron
Other
85 stars 19 forks source link

Changes to allow compatibility with PackageCompiler.jl #83

Closed pengwyn closed 3 years ago

pengwyn commented 3 years ago

I have been making a binary using PackageCompiler.jl and two issues came up:

I've tested this with my program and it successfully ran on both a Linux and a Windows machine without Julia installed. The only slightly fiddly bit was making sure the julia_main function required for PackageCompiler.jl initialised the Application with the packaged main.js file.

davidanthoff commented 3 years ago

@pengwyn cool! I just pushed some commits to master that fix tests, could you merge master into this branch here so that we can hopefully get the tests to pass?

I think moving the artifact path out of the precompile might have a small performance hit, but probably so small that it doesn't matter that much?

For the location of main.js, would the "proper" way to handle that be to move that file into an artifact, and then it would automatically be relocatable? That of course would be way cumbersome for one file right now, but it does make me wonder whether Julia's artifact system should have something like a "local" artifacts folder in a package: just a folder where one can dump files that should automatically be relocated for PackageCompiler.jl scenarios, and where one can get the path to them via the artifact API. @staticfloat, is that something you guys have considered?

Having said that, I think we can merge this as is for now once the tests pass.

staticfloat commented 3 years ago

For the location of main.js, would the "proper" way to handle that be to move that file into an artifact, and then it would automatically be relocatable? That of course would be way cumbersome for one file right now, but it does make me wonder whether Julia's artifact system should have something like a "local" artifacts folder in a package: just a folder where one can dump files that should automatically be relocated for PackageCompiler.jl scenarios, and where one can get the path to them via the artifact API. @staticfloat, is that something you guys have considered?

I'm not sure what you're saying quite makes sense; after PackageCompiler.jl is through with your package, that entire directory may no longer exist. There are special-casings for Artifacts where those files are specifically copied over, but an important part of an Artifact is that it is content-addressed. I'm not sure that it's less work for you to determine the content-hash of your files and communicate that to PackageCompiler.jl than it is to just create an artifact (it's really quite easy):

using Pkg.Artifacts
hash = create_artifact() do dir
    cp(file, dir)
end
bind_artifact!("./Artifacts.toml", "my_file", hash)
pengwyn commented 3 years ago

I think the idea of putting main.js into the artifact would work and I also considered that when making these commits, but I decided against it. I can't remember the exact reason but I thought it would break a simple use case... I wish I'd written that note down now. Maybe the 2nd artifact would work and be better logically, as updates to upstream electron should not care/affect about the main.js implementation used by Electron.jl.

I think moving the artifact path out of the precompile might have a small performance hit, but probably so small that it doesn't matter that much?

Yeah I think the performance hit here is tiny. It is only on the very first Application instantiation, when it looks for the binary, and as that goes through the intermediate get_electron_binary_cmd, the return type there is always forced to be a String in any case.

I'll update the branch on the weekend to be up to date with master.

pengwyn commented 3 years ago

Rebased and tested with my package compiler project!