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

More complete support for unregistered dependencies #1628

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

Conversation

00vareladavid
Copy link
Contributor

@00vareladavid 00vareladavid commented Jan 23, 2020

This adds a source field in the active project for any packages we are tracking by repo or by path. On add/dev/instantiate we crawl over any unregistered dependencies and download as necessary. After we collect all unregistered dependencies, we continue with resolve normally.

close #492 close #1005 supersedes #1088

@00vareladavid 00vareladavid force-pushed the dv_standalone_project branch 2 times, most recently from 2a180ba to 63ceeb6 Compare January 23, 2020 22:36
@codecov

This comment has been minimized.

@tkf
Copy link
Member

tkf commented Jan 27, 2020

How about looking into all manifest files checked in to the repository (e.g., test/Manifest.toml and docs/Manifest.toml)? You can use them if they include the "root" project as a dependency (which then implies that this Manifest file includes the dependencies of the "root" project).

I implemented it in Rogue.add and it is quite useful. (Edit: it was more like a comment to #1088 than this PR)

end
if new_pkg !== nothing
push!(unreg, new_pkg)
collect_unregistered!(ctx, new_pkg, new_git, unreg)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why you have to recurse? In principle, the manifest of pkg is likely to contain all URLs for unregistered dependencies, isn't it? Otherwise, such manifest cannot be instantiated.

Copy link
Member

Choose a reason for hiding this comment

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

What about when you do an add on a url and that package has dependencies on other unregistered packages?

Copy link
Member

Choose a reason for hiding this comment

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

In this case, I guess you'd need to parse the manifest file(s) contained in the newly to-be-added package? But what I was commenting was that, if this new package has a manifest file, it is likely that you don't need to parse the manifest files of dependencies of this newly added package. Or at least you can try to minimize the I/O by stop recursing if all direct and transitive dependencies are found. It could be an unnecessary premature optimization but don't need something like this to prevent infinite recursion anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tkf Perhaps you missed it, but this PR also introduces a source field (as discussed in #492). So there could be no manifest files involved.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification. I was confused since I came to this PR via another PR #1088.

@00vareladavid
Copy link
Contributor Author

Getting very close now.

Here is an example of toggling between tracking a repo and tracking a path:

(demo) pkg> add https://github.com/00vareladavid/Unregistered.jl
   Updating git-repo `https://github.com/00vareladavid/Unregistered.jl`
   Updating registry at `~/.julia/registries/General`
   Updating git-repo `https://github.com/JuliaRegistries/General.git`
  Resolving package versions...
   Updating `/tmp/demo/Project.toml`
   dcb67f36 + Unregistered v0.2.0 `https://github.com/00vareladavid/Unregistered.jl#master`
   Updating `/tmp/demo/Manifest.toml`
   7876af07 + Example v0.5.3
   dcb67f36 + Unregistered v0.2.0 `https://github.com/00vareladavid/Unregistered.jl#master`

(demo) pkg> dev --local Unregistered
    Cloning git-repo `https://github.com/00vareladavid/Unregistered.jl`
  Resolving package versions...
   Updating `/tmp/demo/Project.toml`
   dcb67f36 ~ Unregistered v0.2.0 `https://github.com/00vareladavid/Unregistered.jl#master` ⇒ v0.2.0 `dev/Unregistered`
   Updating `/tmp/demo/Manifest.toml`
   dcb67f36 ~ Unregistered v0.2.0 `https://github.com/00vareladavid/Unregistered.jl#master` ⇒ v0.2.0 `dev/Unregistered`

(demo) pkg> add Unregistered
   Updating git-repo `https://github.com/00vareladavid/Unregistered.jl`
  Resolving package versions...
   Updating `/tmp/demo/Project.toml`
   dcb67f36 ~ Unregistered v0.2.0 `dev/Unregistered` ⇒ v0.2.0 `https://github.com/00vareladavid/Unregistered.jl#master`
   Updating `/tmp/demo/Manifest.toml`
   dcb67f36 ~ Unregistered v0.2.0 `dev/Unregistered` ⇒ v0.2.0 `https://github.com/00vareladavid/Unregistered.jl#master`

And adding a package with multiple unregistered dependencies:

(demo) pkg> add https://github.com/00vareladavid/UnregisteredA
   Updating git-repo `https://github.com/00vareladavid/UnregisteredA`
   Updating git-repo `https://github.com/00vareladavid/UnregisteredD`
   Updating git-repo `https://github.com/00vareladavid/UnregisteredB`
   Updating git-repo `https://github.com/00vareladavid/UnregisteredC`
  Resolving package versions...
   Updating `/tmp/demo/Project.toml`
   b686e103 + UnregisteredA v0.1.0 `https://github.com/00vareladavid/UnregisteredA#master`
   Updating `/tmp/demo/Manifest.toml`
   b686e103 + UnregisteredA v0.1.0 `https://github.com/00vareladavid/UnregisteredA#master`
   4a15487d + UnregisteredB v0.1.0 `https://github.com/00vareladavid/UnregisteredB#master`
   576bedce + UnregisteredC v0.1.0 `https://github.com/00vareladavid/UnregisteredC#master`
   295405fa + UnregisteredD v0.1.0 `https://github.com/00vareladavid/UnregisteredD#master`

@00vareladavid 00vareladavid changed the title [WIP] [RFC] more complete support for unregistered dependencies More complete support for unregistered dependencies Feb 12, 2020
@KristofferC
Copy link
Member

When you think this is ready, it would be good with a bit of a higher level description of what this implements and how it works etc. Just to aid in reviewing it :)

@00vareladavid
Copy link
Contributor Author

The way that this currently works is that, whenever we track a dependency by path or by repo, we add the source url to the source table in the project file. This solves the basic problem of storing the location for unregistered packages.

Whenever Pkg tracks an package by repo/path, we read its project file and see if it declares any dependencies on unregistered packages by checking its source table. We add any unregistered packages to the dependency graph and recurse until we have collected all reachable unregistered dependencies. Once we have collected unregistered dependencies, their versions are fixed, and we can continue resolving the rest of the dependency graph as normal.

When collecting unregistered packages, if the declared source of the package is a local directory and that directory is not a git repo, we fall back to tracking the dependency by path. This fallback allows #1005 to integrate seamlessly with the rest of the scheme.

@00vareladavid
Copy link
Contributor Author

I will rebase this soon. Any feedback for this, even if high level?

@StefanKarpinski
Copy link
Member

My feedback is "doooo it".

@fredrikekre
Copy link
Member

I think the new source field can be discussed a bit more. The original idea (IIRC) was to probe the (checked in) manifest for source. What was wrong with that idea?

@00vareladavid
Copy link
Contributor Author

The goal for this PR is to make the use of unregistered dependencies as seamless as possible. Relying on the presence of manifests has two problems: (1) people have to remember to commit it and (2) we already use the presence of a manifest as a signal to Pkg.test to use that dependency graph.

@fredrikekre
Copy link
Member

Alright. In which cases is Pkg "allowed" to look at the source field? E.g. I don't think it should be allowed when sticking to registered versions for example.

@KristofferC
Copy link
Member

Alright. In which cases is Pkg "allowed" to look at the source field?

Whenever tracking a package by path or url maybe?

@fredrikekre
Copy link
Member

Yea, just wanted to make sure that is how this is implemented. Also; why not store the source for all packages #378 ?

@KristofferC
Copy link
Member

That's in the manifest though. But yeah, that's a good idea as well. Also, I notice that Cargo stores the URL to the registry where the information for the package was retrieved instead of the URL to the git repo. Something to think about perhaps.

@casabre
Copy link

casabre commented Nov 10, 2020

Any progress here? I would love to see that feature backed in the next release 👍

maltezfaria added a commit to WaveProp/WaveProp that referenced this pull request Dec 14, 2020
@TheCedarPrince
Copy link
Member

Bump - any more progress or work here? Saw the checks did not finish...

@kmsherbertvt
Copy link

Was this PR supposed to have been superseded by #3783?

On the one hand:
Issue #492, which seems to have been requesting the same thing this PR seeks to address, has been closed, and @KristofferC asserted there that #3783 solved it.

On the other hand:
A couple people pointed out in #492 that the [sources] documentation indicates it is only used in active environments, not when tracing dependencies:

Note that this information is only used when this environment is active, i.e. it is not used if this project is a package that is being used as a dependency.

Moreover, I just tried it and I still get the "no known versions" error for an unregistered dependency I've always gotten, when trying to add an unregistered package. (And the most recent mention of this PR seems to be due to another group encountering the same problem.)

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.

Developing local packages can be a bit annoying Store some location info for deps in Project.toml
8 participants