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 support for package extensions #3264

Merged
merged 27 commits into from
Dec 7, 2022
Merged

add support for package extensions #3264

merged 27 commits into from
Dec 7, 2022

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Nov 24, 2022

@KristofferC
Copy link
Member Author

Tweaked the printing for glue pkgs in precompilation:

image

@KristofferC
Copy link
Member Author

Status output:

image

Shows you glue packages and glue dependencies and which ones are loaded (in green).

@IanButterworth
Copy link
Member

Awesome. Should there be a + before the enabled glue packages or something, so that it's not just color

@KristofferC
Copy link
Member Author

Yeah, I can add that

docs/src/creating-packages.md Outdated Show resolved Hide resolved
src/API.jl Outdated Show resolved Hide resolved
src/API.jl Outdated Show resolved Hide resolved
src/API.jl Outdated Show resolved Hide resolved
KristofferC and others added 5 commits November 25, 2022 19:14
Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
callback only when glue packages are not supported
```julia
# This symbol is only defined on Julia versions that support glue packages
if isdefined(Base, :get_gluepkg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if isdefined(Base, :get_gluepkg)
if !isdefined(Base, :get_gluepkg)

If I understand correctly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thanks :)

@halleysfifthinc
Copy link

Should this also include a REPL command to add glue packages, or is this experimental enough that the accessibility of a convenient REPL mode isn't desired?

Something like ] add --glue PkgName [Glue($PkgName)] [glue/PkgName] where ungiven optional glue package name and location arguments would be assumed to be "Glue$PkgName" and ProjectDir/glue/PkgName.jl or ProjectDir/glue/PkgName/PkgName.jl, respectively?

@KristofferC
Copy link
Member Author

KristofferC commented Nov 28, 2022

It could be added but it isn't rely necessary for the core functionality so maybe in another PR.

objects from a wide variety of different Julia packages.
Adding all those different Julia packages as dependencies
could be expensive since they would end up getting loaded even if they were never used.
Instead, these code required to plot objects for specific packages can be put into separate files
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Instead, these code required to plot objects for specific packages can be put into separate files
Instead, the code required to plot objects for specific packages can be put into separate files

@@ -53,6 +54,12 @@ struct PkgInfo

# Deps.toml
deps::Dict{VersionRange, Dict{String, UUID}}

# WeakCompat.toml
glue_compat::Dict{VersionRange, Dict{String, VersionSpec}}
Copy link
Member

Choose a reason for hiding this comment

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

So there will be new registry files written for these then? i.e. we'll need to update RegistryTools.jl to support these as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, there already is a branch on LocalRegistry.jl supporting this.

@@ -1127,7 +1179,7 @@ function rm(ctx::Context, pkgs::Vector{PackageSpec}; mode::PackageMode)
# only declare `compat` for remaining direct or `extra` dependencies
# `julia` is always an implicit direct dependency
filter!(ctx.env.project.compat) do (name, _)
name == "julia" || name in keys(ctx.env.project.deps) || name in keys(ctx.env.project.extras)
name == "julia" || name in keys(ctx.env.project.deps) || name in keys(ctx.env.project.extras) || name in keys(ctx.env.project.gluedeps)
Copy link
Member

Choose a reason for hiding this comment

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

I tend to prefer haskey(dict, name) than name in keys(dict)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I just kept the existing way of writing it but that is better indeed.

new = download_source(ctx)
fixup_glue!(ctx.env, pkgs)
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to call fixup_glue! from download_source? Or are there cases where we download, but don't want to fixup glue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. Or have another function that calls both of them.

Before this PR we could create the manifest without even having to download the packages. Now, we need to look up the glue packages in the downloaded packages project file since that information is not stored in the registry.

An alternative to this is to not store the glue package mapping in the manifest at all and just look it up in the package's project file in code loading. But we have so far tried to avoid that opting for storing everything required for code loading in the manifest...

@KristofferC KristofferC changed the title add support for glue packages add support for package extensions Dec 3, 2022
Copy link
Member

@IanButterworth IanButterworth left a comment

Choose a reason for hiding this comment

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

LGTM. Just some minor formatting and wording etc.

docs/src/creating-packages.md Outdated Show resolved Hide resolved
docs/src/creating-packages.md Outdated Show resolved Hide resolved
docs/src/creating-packages.md Outdated Show resolved Hide resolved
src/Pkg.jl Outdated Show resolved Hide resolved
src/Types.jl Outdated Show resolved Hide resolved
src/Operations.jl Outdated Show resolved Hide resolved
KristofferC and others added 6 commits December 7, 2022 19:38
It is possible to think that `Registry` may be a standalone Pkg command with the previous docstring (e.g. a command that constructs or displays registry info).
Co-authored-by: Lilith Hafner <Lilith.Hafner@gmail.com>
`APIOptions` is defined as an alias for `Dict{Symbol, Any}`.
Consequently, the definition of a constructor for `APIOptions`
introduced a new constructor for Dict. This commit removes that
constructor.
`PackageToken` is a Union of multiple struct types. This commit removes
a constructor for `PackageToken` and replaces it with a function
`packagetoken`.

Furthermore, this commit removes `PackageIdentifier` as an alias for
String and replaces that with a struct type, which wraps a String
containing a package identifier.
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.

7 participants