From c6a12b2cd4fb20a8a7a313f277d6a5bb74718429 Mon Sep 17 00:00:00 2001 From: Mathias Lang Date: Wed, 21 Feb 2024 22:13:04 +0100 Subject: [PATCH] Remove the need for intermediate `.zip` file in PackageSupplier 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. --- source/dub/dub.d | 6 ++--- source/dub/packagemanager.d | 25 +++++++++++++------ source/dub/packagesuppliers/fallback.d | 2 +- source/dub/packagesuppliers/filesystem.d | 7 +++--- source/dub/packagesuppliers/maven.d | 7 +++--- source/dub/packagesuppliers/packagesupplier.d | 16 +++++++++--- source/dub/packagesuppliers/registry.d | 8 +++--- source/dub/test/base.d | 2 +- 8 files changed, 43 insertions(+), 30 deletions(-) diff --git a/source/dub/dub.d b/source/dub/dub.d index 871df0fab..69518ab61 100644 --- a/source/dub/dub.d +++ b/source/dub/dub.d @@ -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 diff --git a/source/dub/packagemanager.d b/source/dub/packagemanager.d index 775fcb669..69c7ff9b5 100644 --- a/source/dub/packagemanager.d +++ b/source/dub/packagemanager.d @@ -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)); } @@ -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()); @@ -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 diff --git a/source/dub/packagesuppliers/fallback.d b/source/dub/packagesuppliers/fallback.d index 514517331..a2009d65b 100644 --- a/source/dub/packagesuppliers/fallback.d +++ b/source/dub/packagesuppliers/fallback.d @@ -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); } diff --git a/source/dub/packagesuppliers/filesystem.d b/source/dub/packagesuppliers/filesystem.d index f671abf5d..657f4c27a 100644 --- a/source/dub/packagesuppliers/filesystem.d +++ b/source/dub/packagesuppliers/filesystem.d @@ -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, diff --git a/source/dub/packagesuppliers/maven.d b/source/dub/packagesuppliers/maven.d index 18b8bba2d..bb0a0190c 100644 --- a/source/dub/packagesuppliers/maven.d +++ b/source/dub/packagesuppliers/maven.d @@ -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; diff --git a/source/dub/packagesuppliers/packagesupplier.d b/source/dub/packagesuppliers/packagesupplier.d index 63d83b795..fd7e40d7f 100644 --- a/source/dub/packagesuppliers/packagesupplier.d +++ b/source/dub/packagesuppliers/packagesupplier.d @@ -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) diff --git a/source/dub/packagesuppliers/registry.d b/source/dub/packagesuppliers/registry.d index c46932eea..9ba9ccc13 100644 --- a/source/dub/packagesuppliers/registry.d +++ b/source/dub/packagesuppliers/registry.d @@ -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; diff --git a/source/dub/test/base.d b/source/dub/test/base.d index 46761feb1..19f4be56e 100644 --- a/source/dub/test/base.d +++ b/source/dub/test/base.d @@ -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"