Skip to content

Commit

Permalink
Don't call getPackageIterator from loadGitPackage
Browse files Browse the repository at this point in the history
`getPackageIterator` API forces us to do a refresh, which loads all
packages on the filesystem. However, for `loadGitPackage`'s use case,
we actually can simplify the check quite a bit.

Doing so means that we can now avoid doing a scan for projects
that use SCM-only dependencies.

Test Plan: On master, running the test (without the fix) triggers:
```
core.exception.AssertError@source/dub/test/base.d(790): Trying to access poisoned path: /dub/user/packages/poison/1.0.0/poison/dub.json
```
While with the fix, the test passes.
  • Loading branch information
Geod24 committed Jun 4, 2024
1 parent ee5b2a4 commit fb57522
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 11 deletions.
20 changes: 13 additions & 7 deletions source/dub/packagemanager.d
Original file line number Diff line number Diff line change
Expand Up @@ -464,13 +464,19 @@ class PackageManager {
string gitReference = repo.ref_.chompPrefix("~");
NativePath destination = this.getPackagePath(PlacementLocation.user, name, repo.ref_);

foreach (p; getPackageIterator(name.toString())) {
if (p.path == destination) {
return p;
}
}

if (!this.gitClone(repo.remote, gitReference, destination))
// Before doing a git clone, let's see if the package exists locally
if (this.existsDirectory(destination)) {
// It exists, check if we already loaded it.
// Either we loaded it on refresh and it's in PlacementLocation.user,
// or we just added it and it's in m_internal.
foreach (p; this.m_internal.fromPath)
if (p.path == destination)
return p;
if (this.m_repositories.length)
foreach (p; this.m_repositories[PlacementLocation.user].fromPath)
if (p.path == destination)
return p;
} else if (!this.gitClone(repo.remote, gitReference, destination))
return null;

Package result = this.load(destination);
Expand Down
13 changes: 9 additions & 4 deletions source/dub/test/base.d
Original file line number Diff line number Diff line change
Expand Up @@ -784,6 +784,11 @@ public class FSEntry
{
auto entry = this.lookup(path);
enforce(entry.attributes.type == Type.File, "Trying to read a directory");
// This is a hack to make poisoning a file possible.
// However, it is rather crude and doesn't allow to poison directory.
// Consider introducing a derived type to allow it.
assert(entry.content != "poison".representation,
"Trying to access poisoned path: " ~ path.toNativeString());
return entry.content.dup;
}

Expand All @@ -792,11 +797,11 @@ public class FSEntry
{
import std.utf : validate;

auto entry = this.lookup(path);
enforce(entry.attributes.type == Type.File, "Trying to read a directory");
const content = this.readFile(path);
// Ignore BOM: If it's needed for a test, add support for it.
validate(cast(const(char[])) entry.content);
return cast(string) entry.content.idup();
validate(cast(const(char[])) content);
// `readFile` just `dup` the content, so it's safe to cast.
return cast(string) content;
}

/// Write to this file
Expand Down
23 changes: 23 additions & 0 deletions source/dub/test/other.d
Original file line number Diff line number Diff line change
Expand Up @@ -79,3 +79,26 @@ version "1.0.0"`, PackageFormat.sdl);
const actualDir = newDub.project.getDependency("b", true).path();
assert(actualDir == BDir, actualDir.toNativeString());
}

// Check that SCM-only dependencies don't lead to a scan of the FS
unittest
{
const ValidURL = `git+https://example.com/dlang/dub`;
// Taken from a commit in the dub repository
const ValidHash = "54339dff7ce9ec24eda550f8055354f712f15800";
const Template = `{"name": "%s", "version": "1.0.0", "dependencies": {
"dep1": { "repository": "%s", "version": "%s" }}}`;

scope dub = new TestDub((scope FSEntry fs) {
// This should never be read
fs.writePackageFile("poison", "1.0.0", `poison`);
fs.writeFile(TestDub.ProjectPath ~ "dub.json",
`{ "name": "a", "dependencies": {"b": { "repository": "` ~
ValidURL ~ `", "version": "` ~ ValidHash ~ `" }} }`);
});
dub.packageManager.addTestSCMPackage(
Repository(ValidURL, ValidHash), `{"name":"b"}`);

dub.loadPackage();
assert(dub.project.hasAllDependencies());
}

0 comments on commit fb57522

Please sign in to comment.