Skip to content

Commit

Permalink
Remove the need for intermediate .zip file in PackageSupplier
Browse files Browse the repository at this point in the history
By using a different overload of `retryDownload`,
we return the data instead of writing to disk,
ensuring that we don't have a lingering file
in memory and reducing IO.

This follow the same approach for deprecations as previous
`PackageSupplier` deprecations: it's an inevitable, but
easily fixable, breaking change for classes that implement
`PackageSupplier`, but it's a proper deprecation for client
code, based on the assumption that all the classes implementing
`PackageSupplier` are in Dub.
  • Loading branch information
Geod24 committed Feb 22, 2024
1 parent 794982d commit c6a12b2
Show file tree
Hide file tree
Showing 8 changed files with 43 additions and 30 deletions.
6 changes: 2 additions & 4 deletions source/dub/dub.d
Original file line number Diff line number Diff line change
Expand Up @@ -1021,13 +1021,11 @@ class Dub {
{
import std.zip : ZipException;

auto path = getTempFile(name.main.toString(), ".zip");
supplier.fetchPackage(path, name.main, range, (options & FetchOptions.usePrerelease) != 0); // Q: continue on fail?
scope(exit) removeFile(path);
auto data = supplier.fetchPackage(name.main, range, (options & FetchOptions.usePrerelease) != 0); // Q: continue on fail?
logDiagnostic("Placing to %s...", location.toString());

try {
return m_packageManager.store(path, location, name.main, ver);
return m_packageManager.store(data, location, name.main, ver);
} catch (ZipException e) {
logInfo("Failed to extract zip archive for %s@%s...", name, ver);
// re-throw the exception at the end of the loop
Expand Down
25 changes: 18 additions & 7 deletions source/dub/packagemanager.d
Original file line number Diff line number Diff line change
Expand Up @@ -738,7 +738,9 @@ class PackageManager {
deprecated("Use `store(NativePath source, PlacementLocation dest, string name, Version vers)`")
Package storeFetchedPackage(NativePath zip_file_path, Json package_info, NativePath destination)
{
return this.store_(zip_file_path, destination,
import dub.internal.vibecompat.core.file;

return this.store_(readFile(zip_file_path), destination,
PackageName(package_info["name"].get!string),
Version(package_info["version"].get!string));
}
Expand Down Expand Up @@ -776,6 +778,16 @@ class PackageManager {
{
import dub.internal.vibecompat.core.file;

auto data = readFile(src);
return this.store(data, dest, name, vers);
}

/// Ditto
Package store(ubyte[] data, PlacementLocation dest,
in PackageName name, in Version vers)
{
import dub.internal.vibecompat.core.file;

assert(!name.sub.length, "Cannot store a subpackage, use main package instead");
NativePath dstpath = this.getPackagePath(dest, name, vers.toString());
ensureDirectory(dstpath.parentPath());
Expand All @@ -787,26 +799,25 @@ class PackageManager {
if (dstpath.existsFile()) {
return this.getPackage(name, vers, dest);
}
return this.store_(src, dstpath, name, vers);
return this.store_(data, dstpath, name, vers);
}

/// Backward-compatibility for deprecated overload, simplify once `storeFetchedPatch`
/// is removed
private Package store_(NativePath src, NativePath destination,
private Package store_(ubyte[] data, NativePath destination,
in PackageName name, in Version vers)
{
import dub.internal.vibecompat.core.file;
import std.range : walkLength;

logDebug("Placing package '%s' version '%s' to location '%s' from file '%s'",
name, vers, destination.toNativeString(), src.toNativeString());
logDebug("Placing package '%s' version '%s' to location '%s'",
name, vers, destination.toNativeString());

enforce(!existsFile(destination),
"%s (%s) needs to be removed from '%s' prior placement."
.format(name, vers, destination));

logDebug("Opening file %s", src);
ZipArchive archive = new ZipArchive(readFile(src));
ZipArchive archive = new ZipArchive(data);
logDebug("Extracting from zip.");

// In a GitHub zip, the actual contents are in a sub-folder
Expand Down
2 changes: 1 addition & 1 deletion source/dub/packagesuppliers/fallback.d
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ package abstract class AbstractFallbackPackageSupplier : PackageSupplier

// Workaround https://issues.dlang.org/show_bug.cgi?id=2525
abstract override Version[] getVersions(in PackageName name);
abstract override void fetchPackage(in NativePath path, in PackageName name, in VersionRange dep, bool pre_release);
abstract override ubyte[] fetchPackage(in PackageName name, in VersionRange dep, bool pre_release);
abstract override Json fetchPackageRecipe(in PackageName name, in VersionRange dep, bool pre_release);
abstract override SearchResult[] searchPackages(string query);
}
Expand Down
7 changes: 3 additions & 4 deletions source/dub/packagesuppliers/filesystem.d
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,14 @@ class FileSystemPackageSupplier : PackageSupplier {
return ret;
}

override void fetchPackage(in NativePath path, in PackageName name,
override ubyte[] fetchPackage(in PackageName name,
in VersionRange dep, bool pre_release)
{
import dub.internal.vibecompat.core.file : copyFile, existsFile;
enforce(path.absolute);
import dub.internal.vibecompat.core.file : readFile, existsFile;
logInfo("Storing package '%s', version requirements: %s", name.main, dep);
auto filename = bestPackageFile(name, dep, pre_release);
enforce(existsFile(filename));
copyFile(filename, path);
return readFile(filename);
}

override Json fetchPackageRecipe(in PackageName name, in VersionRange dep,
Expand Down
7 changes: 3 additions & 4 deletions source/dub/packagesuppliers/maven.d
Original file line number Diff line number Diff line change
Expand Up @@ -47,21 +47,20 @@ class MavenRegistryPackageSupplier : PackageSupplier {
return ret;
}

override void fetchPackage(in NativePath path, in PackageName name,
override ubyte[] fetchPackage(in PackageName name,
in VersionRange dep, bool pre_release)
{
import std.format : format;
auto md = getMetadata(name.main);
Json best = getBestPackage(md, name.main, dep, pre_release);
if (best.type == Json.Type.null_)
return;
return null;
auto vers = best["version"].get!string;
auto url = m_mavenUrl ~ NativePath(
"%s/%s/%s-%s.zip".format(name.main, vers, name.main, vers));

try {
retryDownload(url, path, 3, httpTimeout);
return;
return retryDownload(url, 3, httpTimeout);
}
catch(HTTPStatusException e) {
if (e.status == 404) throw e;
Expand Down
16 changes: 12 additions & 4 deletions source/dub/packagesuppliers/packagesupplier.d
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,25 @@ interface PackageSupplier {
Version[] getVersions(in PackageName name);


/** Downloads a package and stores it as a ZIP file.
/** Downloads a package and returns its binary content
Params:
path = Absolute path of the target ZIP file
name = Name of the package to retrieve
dep = Version constraint to match against
pre_release = If true, matches the latest pre-release version.
Otherwise prefers stable versions.
*/
void fetchPackage(in NativePath path, in PackageName name,
in VersionRange dep, bool pre_release);
ubyte[] fetchPackage(in PackageName name, in VersionRange dep,
bool pre_release);

deprecated("Use `writeFile(path, fetchPackage(PackageName, VersionRange, bool))` instead")
final void fetchPackage(in NativePath path, in PackageName name,
in VersionRange dep, bool pre_release)
{
import dub.internal.vibecompat.core.file : writeFile;
if (auto res = this.fetchPackage(name, dep, pre_release))
writeFile(path, res);
}

deprecated("Use `fetchPackage(NativePath, PackageName, VersionRange, bool)` instead")
final void fetchPackage(NativePath path, string name, Dependency dep, bool pre_release)
Expand Down
8 changes: 3 additions & 5 deletions source/dub/packagesuppliers/registry.d
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,15 @@ class RegistryPackageSupplier : PackageSupplier {
return ret;
}

override void fetchPackage(in NativePath path, in PackageName name,
override ubyte[] fetchPackage(in PackageName name,
in VersionRange dep, bool pre_release)
{
import std.format : format;

auto url = genPackageDownloadUrl(name, dep, pre_release);
if(url.isNull)
return;
if(url.isNull) return null;
try {
retryDownload(url.get, path);
return;
return retryDownload(url.get);
}
catch(HTTPStatusException e) {
if (e.status == 404) throw e;
Expand Down
2 changes: 1 addition & 1 deletion source/dub/test/base.d
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ public class MockPackageSupplier : PackageSupplier
}

///
public override void fetchPackage(in NativePath path, in PackageName name,
public override ubyte[] fetchPackage(in PackageName name,
in VersionRange dep, bool pre_release)
{
assert(0, "%s - fetchPackage not implemented for: %s"
Expand Down

0 comments on commit c6a12b2

Please sign in to comment.