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

Add a PackageCompiler plugin #290

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ffevotte
Copy link

This PR adds a PackageCompiler plugin which sets up sysimage generation using PackageCompiler.jl.

This follows the same kind of setup as Documenter: a build subdirectory is created, that defines its own environment in which PackageCompiler is Pkg.added, and the main package is Pkg.deved.

The user can choose between several options to determine the list of packages to be included in the sysimage:

  • all dependencies of the package, or
  • the package itself, or
  • a list of explicitly specified packages.

I'm not sure whether this follows best practices, and I'm willing to make any change if something can be improved.

@codecov
Copy link

codecov bot commented Apr 11, 2021

Codecov Report

Merging #290 (3f8336e) into master (4302ecb) will increase coverage by 0.00%.
The diff coverage is 94.73%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #290   +/-   ##
=======================================
  Coverage   94.40%   94.41%           
=======================================
  Files          20       21    +1     
  Lines         608      627   +19     
=======================================
+ Hits          574      592   +18     
- Misses         34       35    +1     
Impacted Files Coverage Δ
src/PkgTemplates.jl 100.00% <ø> (ø)
src/plugin.jl 98.38% <ø> (ø)
src/plugins/package_compiler.jl 94.73% <94.73%> (ø)

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 4302ecb...3f8336e. Read the comment docs.

Copy link
Collaborator

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

Thanks for opening this!

)

Sets up sysimage generation via [PackageCompiler.jl](https://github.com/JuliaLang/PackageCompiler.jl).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does PackageCompiler.jl require a certain minimum Julia version?
If so, should we document that here? e.g. in a !!! note block?

Copy link
Contributor

Choose a reason for hiding this comment

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

)

"""
PackageCompiler(;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to include this docstring in the docs, e.g. on docs/src/user.md in the Misc section

using PackageCompiler

# List of packages to include in the sysimage
{{#SYSIMAGE_DEPS}}packages = Symbol.(keys(Pkg.project().dependencies)) # or packages = [:Plots, :DataFrames]
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think we can use this because Pkg.project().dependencies is only defined in Julia v1.4+, and is not stable API anyway.

?Pkg.project says:

This feature requires Julia 1.4, and is considered experimental.

And we want to support as many v1.x versions as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

PkgDeps might be an alternative solution https://github.com/JuliaEcosystem/PkgDeps.jl (although the min Julia version there is 1.5)

make_jl::String = default_file("build", "make.jl")
precompile_jl::String = default_file("build", "precompile.jl")
sysimage_name::String = "sysimage"
packages::Union{Symbol, AbstractVector} = :deps
Copy link
Collaborator

Choose a reason for hiding this comment

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

creat_sysimage seems to require packages names be Symbol, so let's just support that (https://julialang.github.io/PackageCompiler.jl/dev/refs/#PackageCompiler.create_sysimage)

Suggested change
packages::Union{Symbol, AbstractVector} = :deps
packages::Union{Symbol, Vector{Symbol}} = :deps

```
# Explicitly list packages to include into the sysimage
PackageCompiler(packages = [:Plots, :DataFrames])
PackageCompiler(packages = ["Plots", "DataFrames"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
PackageCompiler(packages = ["Plots", "DataFrames"])


- `:deps`: include in the sysimage all direct dependencies of the package.
- `:pkg`: include in the sysimage the package itself.
- vector of package names, as strings or symbols: include all listed packages into the sysimage.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- vector of package names, as strings or symbols: include all listed packages into the sysimage.
- `Vector{Symbol}` of package names: include all listed packages into the sysimage.

included in the sysimage. Supported values are:

- `:deps`: include in the sysimage all direct dependencies of the package.
- `:pkg`: include in the sysimage the package itself.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- `:pkg`: include in the sysimage the package itself.
- `:pkg`: include in the sysimage only the package itself.

The `packages` keyword argument allows specifying which packages should be
included in the sysimage. Supported values are:

- `:deps`: include in the sysimage all direct dependencies of the package.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we can robustly support this option... is there anything in the PackageCompiler docs on using a pattern like this?

@nickrobinson251 nickrobinson251 added the Plugin Idea A proposal for a feature that can be accomplished with a plugin label Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Plugin Idea A proposal for a feature that can be accomplished with a plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants