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.

Sign up for GitHub

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

WIP: Search the active project for artifacts #1727

Closed
wants to merge 7 commits into from
+56 −16
Closed

WIP: Search the active project for artifacts #1727

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
98493a3
Search the active project for artifacts
DilumAluthge Mar 21, 2020
6e575c8
Search the project first
DilumAluthge Mar 22, 2020
278c915
Update Artifacts.jl
DilumAluthge Mar 22, 2020
fce5435
Update Artifacts.jl
DilumAluthge Mar 22, 2020
bcb0e79
Update Artifacts.jl
DilumAluthge Mar 22, 2020
6a8fb89
Update API.jl
DilumAluthge Mar 22, 2020
8e58ab9
Update API.jl
DilumAluthge Mar 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/Artifacts.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,14 @@ current set of depot paths and the current artifact directory override via the m
"""
function artifacts_dirs(args...)
if ARTIFACTS_DIR_OVERRIDE[] === nothing
return [abspath(depot, "artifacts", args...) for depot in depots()]
depot_artifacts = [abspath(depot, "artifacts", args...) for depot in depots()] # search all depots
project = Base.active_project()
if project === nothing || isempty(project)
project_artifacts = String[]
else
project_artifacts = [abspath(dirname(Base.active_project()), "artifacts", args...)] # search the active project
end
return vcat(depot_artifacts, project_artifacts)
Copy link
Member

Choose a reason for hiding this comment

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

Switching the order seems better—might as well look in the project first.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that, but here's the problem. If we put the project first, then, if the artifact needs to be downloaded, it will always be downloaded into the project.

Copy link
Member Author

Choose a reason for hiding this comment

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

But for most users, we want to download artifacts into the first depot.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we could return them in this order when searching/reading:

  • project first, then depots

But return them in this order when downloading/creating/writing:

  • depots first, then project

But... this might make things more confusing?

Copy link
Contributor

@oxinabox oxinabox Mar 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It may be interesting to look at DataDeps.jl's solution which is basically what you propose.
https://white.ucc.asn.au/DataDeps.jl/stable/z10-for-end-users/#The-Load-Path-1
Basically the search order starts looking in project, then continues to more and more general (it allows many locations).
the save order attempts to save in the same list of locations (since by design it can fail, and want to move on to next), but skips the project specific one at the top.

however. that is almost completely irrelevent for you.
Search order doesn't matter because content addressing promises that no matter where you find a match, you know that match is good and is identical to anywhere else you might find a match.

Copy link
Member

Choose a reason for hiding this comment

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

With artifacts, search order doesn't matter for correctness, (since it's content-addressed, if you find it, it's the right thing) but it could matter for performance. IMO you're more likely to have something sitting in your overall location (project-local deps will not be the norm, I don't think) so keeping it this way is better, IMO.

Copy link
Member

@StefanKarpinski StefanKarpinski Mar 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is another reason why treating the project as a depot more generally is wrong: it should only be a place you look in when loading artifacts. If it's not there, you download in the normal fashion into the standard user depot. Looking in $project/artifacts first is absolutely the right thing to do, however: if the project has bothered to vendor its dependencies, we should use those vendored dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

@staticfloat, I don't understand the performance consideration. Why would the system copy of an artifact be better than the project copy?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, to be clear, the plan is that we will prepend the project to the front of the list of search paths when we are searching for an artifact. But when we are creating artifacts, we are going to use the user depot, correct?

Copy link
Member

Choose a reason for hiding this comment

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

@staticfloat, I don't understand the performance consideration. Why would the system copy of an artifact be better than the project copy?

It's literally just saving a stat() or two. It's not the performance of the binaries themselves, but merely how quickly you find them. I don't think it's a big issue, even on Windows.

else
# If we've been given an override, use _only_ that directory.
return [abspath(ARTIFACTS_DIR_OVERRIDE[], args...)]
Expand Down