Skip to content

Commit

Permalink
Deprecate Package.load, use PackageManager.load instead
Browse files Browse the repository at this point in the history
In order for the dependency injection approach to work,
we need to be able to override every single instance of
`Package.load`. We could not override `Package.load` itself
as it is a static function.
However, in the process of adding support for path-based dependencies
to the test suite, it became obvious that the functionality offered
by `Package.load` is actually better suited for the `PackageManager`.
Deprecating `Package.load` confirms this: Not a single call was outside
of `PackageManager`, and no extra parameter had to be provided anywhere
to make this work.
This also nicely simplifies the `TestPackageManager` implementation,
as we can now overload `load` and let `getPackage` alone.
  • Loading branch information
Geod24 committed Jan 2, 2024
1 parent 92da402 commit 834c105
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 29 deletions.
4 changes: 3 additions & 1 deletion source/dub/package_.d
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,10 @@ static immutable string[] builtinBuildTypes = [
/** Represents a package, including its sub packages.
*/
class Package {
// `package` visibility as it is set from the PackageManager
package NativePath m_infoFile;
private {
NativePath m_path;
NativePath m_infoFile;
PackageRecipe m_info;
PackageRecipe m_rawRecipe;
Package m_parentPackage;
Expand Down Expand Up @@ -173,6 +174,7 @@ class Package {
determined by invoking the VCS (GIT currently).
mode = Whether to issue errors, warning, or ignore unknown keys in dub.json
*/
deprecated("Use `PackageManager.getOrLoadPackage` instead of loading packages directly")
static Package load(NativePath root, NativePath recipe_file = NativePath.init,
Package parent = null, string version_override = "",
StrictMode mode = StrictMode.Ignore)
Expand Down
59 changes: 51 additions & 8 deletions source/dub/packagemanager.d
Original file line number Diff line number Diff line change
Expand Up @@ -332,11 +332,54 @@ class PackageManager {
foreach (p; this.m_internal.fromPath)
if (p.path == path && (!p.parentPackage || (allow_sub_packages && p.parentPackage.path != p.path)))
return p;
auto pack = Package.load(path, recipe_path, null, null, mode);
auto pack = this.load(path, recipe_path, null, null, mode);
addPackages(this.m_internal.fromPath, pack);
return pack;
}

/**
* Loads a `Package` from the filesystem
*
* This is called when a `Package` needs to be loaded from the path.
* This does not change the internal state of the `PackageManager`,
* it simply loads the `Package` and returns it - it is up to the caller
* to call `addPackages`.
*
* Throws:
* If no package can be found at the `path` / with the `recipe`.
*
* Params:
* path = The directory in which the package resides.
* recipe = Optional path to the package recipe file. If left empty,
* the `path` directory will be searched for a recipe file.
* parent = Reference to the parent package, if the new package is a
* sub package.
* version_ = Optional version to associate to the package instead of
* the one declared in the package recipe, or the one
* determined by invoking the VCS (GIT currently).
* mode = Whether to issue errors, warning, or ignore unknown keys in
* dub.json
*
* Returns: A populated `Package`.
*/
protected Package load(NativePath path, NativePath recipe = NativePath.init,
Package parent = null, string version_ = null,
StrictMode mode = StrictMode.Ignore) const
{
if (recipe.empty)
recipe = Package.findPackageFile(path);

enforce(!recipe.empty,
"No package file found in %s, expected one of %s"
.format(path.toNativeString(),
packageInfoFiles.map!(f => cast(string)f.filename).join("/")));

auto content = readPackageRecipe(recipe, parent ? parent.name : null, mode);
auto ret = new Package(content, path, parent, version_);
ret.m_infoFile = recipe;
return ret;
}

/** For a given SCM repository, returns the corresponding package.
An SCM repository is provided as its remote URL, the repository is cloned
Expand Down Expand Up @@ -399,7 +442,7 @@ class PackageManager {
return null;
}

return Package.load(destination);
return this.load(destination);
}

/**
Expand Down Expand Up @@ -787,7 +830,7 @@ symlink_exit:
logDebug("%s file(s) copied.", to!string(countFiles));

// overwrite dub.json (this one includes a version field)
auto pack = Package.load(destination, NativePath.init, null, vers.toString());
auto pack = this.load(destination, NativePath.init, null, vers.toString());

if (pack.recipePath.head != defaultPackageFilename)
// Storeinfo saved a default file, this could be different to the file from the zip.
Expand Down Expand Up @@ -850,7 +893,7 @@ symlink_exit:
this.refresh();

path.endsWithSlash = true;
auto pack = Package.load(path);
auto pack = this.load(path);
enforce(pack.name.length, "The package has no name, defined in: " ~ path.toString());
if (verName.length)
pack.version_ = Version(verName);
Expand Down Expand Up @@ -995,7 +1038,7 @@ symlink_exit:
p.normalize();
enforce(!p.absolute, "Sub package paths must be sub paths of the parent package.");
auto path = pack.path ~ p;
sp = Package.load(path, NativePath.init, pack);
sp = this.load(path, NativePath.init, pack);
} else sp = new Package(spr.recipe, pack.path, pack);

// Add the sub-package.
Expand Down Expand Up @@ -1241,7 +1284,7 @@ package struct Location {

if (!pp) {
auto infoFile = Package.findPackageFile(path);
if (!infoFile.empty) pp = Package.load(path, infoFile);
if (!infoFile.empty) pp = manager.load(path, infoFile);
else {
logWarn("Locally registered package %s %s was not found. Please run 'dub remove-local \"%s\"'.",
name, ver, path.toNativeString());
Expand Down Expand Up @@ -1304,7 +1347,7 @@ package struct Location {
p = pp;
break;
}
if (!p) p = Package.load(pack_path, packageFile);
if (!p) p = mgr.load(pack_path, packageFile);
mgr.addPackages(this.fromPath, p);
} catch (ConfigException exc) {
// Configy error message already include the path
Expand Down Expand Up @@ -1403,7 +1446,7 @@ package struct Location {
return null;

logDiagnostic("Lazily loading package %s:%s from %s", lookupName, vers, path);
auto p = Package.load(path);
auto p = mgr.load(path);
enforce(
p.version_ == vers,
format("Package %s located in %s has a different version than its path: Got %s, expected %s",
Expand Down
30 changes: 10 additions & 20 deletions source/dub/test/base.d
Original file line number Diff line number Diff line change
Expand Up @@ -289,27 +289,17 @@ package class TestPackageManager : PackageManager
// Do nothing
}

/**
* Looks up a specific package
*
* Unlike its parent class, no lazy loading is performed.
* Additionally, as they are already deprecated, overrides are
* disabled and not available.
*/
public override Package getPackage(string name, Version vers, bool enable_overrides = false)
/**
* Loads a `Package`
*
* This is currently not implemented, and any call to it will trigger
* an assert, as that would otherwise be an access to the filesystem.
*/
protected override Package load(NativePath path, NativePath recipe = NativePath.init,
Package parent = null, string version_ = null,
StrictMode mode = StrictMode.Ignore) const
{
//assert(!enable_overrides, "Overrides are not implemented for TestPackageManager");

// Implementation inspired from `PackageManager.lookup`,
// except we replaced `load` with `lookup`.
if (auto pkg = this.m_internal.lookup(name, vers, this))
return pkg;

foreach (ref location; this.m_repositories)
if (auto p = location.lookup(name, vers, this))
return p;

return null;
assert(0, "`TestPackageManager.load` is not implemented");
}

/**
Expand Down

0 comments on commit 834c105

Please sign in to comment.