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

loading: Don't assume a package is loaded by its own name #55162

Closed
wants to merge 1 commit into from

Conversation

Keno
Copy link
Member

@Keno Keno commented Jul 18, 2024

Recent discussion on slack has revealed that there is some disagreement about what it means for a package to have a different name in the deps section for one of its dependencies than said dependency itself declares in its own Project.toml.

There's basically two views:

  1. This situation shouldn't arise, the primary purpose of the deps section is to disambiguate between two packages of the same name (but with different UUIDs).

  2. The Project.toml file declares a mapping name => uuid that all loading operations within said package operate on and the actual name of the package is largely irrelevant.

Worse, the current code doesn't really do either. It mostly behaves like the second, except that it doesn't actually work, because it forms the default entrypath using the name from the deps section, not the Project.toml (resp. a copy of it in the Manifest.toml). As a result, the only time you're allowed to use a name other than what the project declares in a [deps] section is when the dependency explicitly declares an entrypath (which is a relatively new feature).

This PR adjusts loading to fully behave like the second interpretation. For example, having the following in

Project.toml

[deps]
GeneralHTTP = "cd3eb016-35fb-5094-929b-558a96fad6f3"
[[deps.GeneralHTTP]]
name = "HTTP"
deps = ["Base64", "CodecZlib", "ConcurrentUtilities", "Dates", "ExceptionUnwrapping", "Logging", "LoggingExtras", "MbedTLS", "NetworkOptions", "OpenSSL", "Random", "SimpleBufferStream", "Sockets", "URIs", "UUIDs"]
git-tree-sha1 = "d1d712be3164d61d1fb98e7ce9bcbc6cc06b45ed"
uuid = "cd3eb016-35fb-5094-929b-558a96fad6f3"
version = "1.10.8"

would allow using GeneralHTTP to load the package otherwise known as HTTP.

This is only the loading side of this. While Pkg seems surprisingly ok with this also, it is definitely not tested and likely to cause confusing behavior, so the primary purpose of this PR is to decide which behavior we think is correct and then to gradually bring the implementation into compliance.

Recent discussion on slack has revealed that there is some disagreement
about what it means for a package to have a different name in the
`deps` section for one of its dependencies than said dependency itself
declares in its own Project.toml.

There's basically two views:
1. This situation shouldn't arise, the primary purpose of the deps section is
   to disambiguate between two packages of the same name (but with different
   UUIDs).

2. The `Project.toml` file declares a mapping `name` => `uuid` that all loading
   operations within said package operate on and the actual name of the package
   is largely irrelevant.

Worse, the current code doesn't really do either. It mostly behaves like the
second, except that it doesn't actually work, because it forms the default
entrypath using the name from the deps section, not the Project.toml (resp.
a copy of it in the Manifest.toml). As a result, the only time you're allowed
to use a name other than what the project declares in a `[deps]` section
is when the dependency explicitly declares an `entrypath` (which is a relatively
new feature).

This PR adjusts loading to fully behave like the second interpretation.
For example, having the following in

Project.toml
```
[deps]
GeneralHTTP = "cd3eb016-35fb-5094-929b-558a96fad6f3"
```

```
[[deps.GeneralHTTP]]
name = "HTTP"
deps = ["Base64", "CodecZlib", "ConcurrentUtilities", "Dates", "ExceptionUnwrapping", "Logging", "LoggingExtras", "MbedTLS", "NetworkOptions", "OpenSSL", "Random", "SimpleBufferStream", "Sockets", "URIs", "UUIDs"]
git-tree-sha1 = "d1d712be3164d61d1fb98e7ce9bcbc6cc06b45ed"
uuid = "cd3eb016-35fb-5094-929b-558a96fad6f3"
version = "1.10.8"
```

would allow `using GeneralHTTP` to load the package otherwise known
as `HTTP`.

This is only the loading side of this. While Pkg seems surprisingly
ok with this also, it is definitely not tested and likely to cause
confusing behavior, so the primary purpose of this PR is to decide
which behavior we think is correct and then to gradually bring the
implementation into compliance.
@Keno Keno force-pushed the kf/loadingrename branch from 3efd0ad to 04c86bd Compare July 18, 2024 12:19
@ericphanson
Copy link
Contributor

I'm concerned that allowing users to choose arbitrary names in their Project.toml for their dependencies is potentially too expressive and can make reading/understanding code harder. It also would confuse crude static analysis tools. It's an extra degree of freedom that feels unnecessary. However I could see how it could be useful in some situations.

@nsajko nsajko added the packages Package management and loading label Jul 18, 2024
@KristofferC
Copy link
Member

Ref JuliaLang/Pkg.jl#3853

@Keno Keno added the triage This should be discussed on a triage call label Jul 28, 2024
@Keno
Copy link
Member Author

Keno commented Jul 28, 2024

I'm gonna put triage on this. Doesn't have to be the the literal triage call, but just want to remember to discuss this.

@vtjnash
Copy link
Member

vtjnash commented Aug 12, 2024

Closes #29654?

@Keno
Copy link
Member Author

Keno commented Aug 12, 2024

One variant of it anyway. The name of the module still has to match the name of the package at definition time, this just allows it to be imported by another name.

@LilithHafner
Copy link
Member

From triage: make it an error for now and then reconsider if folks really want to support aliased package names (e.g. when renaming upon open sourcing a package)

@LilithHafner LilithHafner removed the triage This should be discussed on a triage call label Aug 15, 2024
@StefanKarpinski
Copy link
Member

There's an older issue about this somewhere... The original intention was to be able to load and use a package by a different name than it's "internal" or "canonical" name. In particular, it would be good to smoothly support changing a package's name, which implies that different versions with the same UUID could have different names and that ought to work. I did not get that working in the initial Julia 1.0 release and it's never been finished, so the tooling definitely doesn't support this and maybe it's a bad idea. After all, you can just change both the name and the UUID, which is what we do in practice, but it seems like conceptually it's good to make the UUID the definitive identity key and have name be essentially incidental and for human use, and if that's the case then it should be straightforward to use a package by a different name than it internally uses for itself. This wouldn't just be a gimmick, it would allow using two different packages with the same name in the same project file, which is currently impossible. You would just pick different names for them in the deps section of the project file and be able to load them by those non-canonical names.

@LilithHafner
Copy link
Member

One consideration that made folks on triage resistant to quiet renaming in the [deps] section is that it makes dependency auditing harder. For example, this package depends on WatchJuliaBurn:

name = "MyPackage"
uuid = "824d21be-5319-a50a-38a1-6b3eba399b47"
version = "1.0.0"


[deps]
Random = "952999c0-2bc0-44cb-b633-dc1e299b187b"

I want any package that directly depends on XZ_jll.jl to include the string "XZ_jll" somewhere in it's Project.toml

@Keno
Copy link
Member Author

Keno commented Aug 15, 2024

I've thought about it some more, but an error here doesn't actually provide as much protection as people thought, because the name of the dependee package might be under the control of a malicious actor. What you really want is a linting pass that makes sure that the name in the deps matches the name in the registry. The other issue is that I was thinking about the loading implications, but the error that people want isn't really implementable, since all we can do in loading is check consistency between the Project and the Manifest

@giordano giordano deleted the kf/loadingrename branch October 7, 2024 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants