Place the extraction dir into the package base dir

Now the "downloads" constant isn't required.

crates/typst-kit/src/package.rs

@@ -20,9 +20,6 @@ pub const DEFAULT_NAMESPACE: &str = "preview";
 /// The default packages sub directory within the package and package cache paths.
 pub const DEFAULT_PACKAGES_SUBDIR: &str = "typst/packages";
 
-/// The default download directory where all packages are downloaded to.
-pub const DEFAULT_DOWNLOAD_DIR: &str = "typst/downloads";
-
 /// Holds information about where packages should be stored and downloads them
 /// on demand, if possible.
 #[derive(Debug)]
@@ -87,7 +84,6 @@ impl PackageStorage {
         progress: &mut dyn Progress,
     ) -> PackageResult<PathBuf> {
         let subdir = format!("{}/{}/{}", spec.namespace, spec.name, spec.version);
-        let download_dir = dirs::cache_dir().map(|dir| dir.join(DEFAULT_DOWNLOAD_DIR));
 
         if let Some(packages_dir) = &self.package_path {
             let dir = packages_dir.join(&subdir);
@@ -96,9 +92,7 @@ impl PackageStorage {
             }
         }
 
-        if let (Some(cache_dir), Some(download_dir)) =
+        if let Some(cache_dir) = &self.package_cache_path {
-            (&self.package_cache_path, download_dir)
-        {
             let dir = cache_dir.join(&subdir);
             if dir.exists() {
                 return Ok(dir);
@@ -106,7 +100,8 @@ impl PackageStorage {
 
             // Download from network if it doesn't exist yet.
             if spec.namespace == DEFAULT_NAMESPACE {
-                self.download_package(&download_dir, spec, &dir, progress)?;
+                let package_base_dir = cache_dir.join(&*spec.namespace);
+                self.download_package(&package_base_dir, spec, &dir, progress)?;
                 if dir.exists() {
                     return Ok(dir);
                 }
@@ -172,7 +167,7 @@ impl PackageStorage {
     /// Panics if the package spec namespace isn't `DEFAULT_NAMESPACE`.
     pub fn download_package(
         &self,
-        download_dir: &Path,
+        package_base_dir: &Path,
         spec: &PackageSpec,
         package_dir: &Path,
         progress: &mut dyn Progress,
@@ -198,13 +193,15 @@ impl PackageStorage {
             }
         };
 
-        // Temporary place where the package will be downloaded before being moved
+        // Temporary place where the package will be extracted before being
-        // to the target directory;
+        // moved to the target directory.
-        let package_download_dir = download_dir.join(format!(
+        //
-            "{}-{}-{}",
+        // To avoid name clashing we use PRNG to get unique directory name.
+        let extracted_package_dir = package_base_dir.join(format!(
+            ".cache-{}-{}-{}",
             spec.name,
             spec.version,
-            rand::random::<u32>() // Make directory name unique.
+            rand::random::<u32>()
         ));
 
         let create_dir = |dir, dir_name| {
@@ -215,20 +212,20 @@ impl PackageStorage {
             })
         };
 
-        create_dir(package_download_dir.as_path(), "download")?;
+        create_dir(extracted_package_dir.as_path(), "temporary package")?;
         create_dir(package_dir, "package")?;
 
         let removed_download_dir = || {
-            fs::remove_dir_all(&package_download_dir).map_err(|err| {
+            fs::remove_dir_all(&extracted_package_dir).map_err(|err| {
                 PackageError::Other(Some(eco_format!(
-                    "failed to delete temporary package directory: {err}"
+                    "failed to remove a temporary package directory: {err}"
                 )))
             })
         };
 
         let decompressed = flate2::read::GzDecoder::new(data.as_slice());
         tar::Archive::new(decompressed)
-            .unpack(&package_download_dir)
+            .unpack(&extracted_package_dir)
             .map_err(|err| {
                 let message = match removed_download_dir() {
                     Err(PackageError::Other(Some(str))) => eco_format!("{err}\n{str}"),
@@ -239,15 +236,16 @@ impl PackageStorage {
 
         // As to not overcomplicate the code base to combat an already rare case
         // where multiple instances try to download the same package version
-        // concurrently, we are abusing the behavior of the `rename` FS
+        // concurrently, we are abusing the behavior of the `rename` FS operation
-        // operation without the help of file locking.
+        // without the help of file locking.
         //
         // From the function's documentation it is stated that:
         //
         // > This will not work if the new name is on a different mount point.
         //
-        // Hence, we assume that the `package_download_dir` and `package_dir`
+        // By extracting package into the same base directory where all packages
-        // are on the same mount point.
+        // live we are (trying our best) making sure that `extracted_package_dir`
+        // and `package_dir` are on the same mount point.
         //
         // When trying to move (i.e., `rename`) directory from one place to
         // another and the target/destination directory name is empty, then the
@@ -261,7 +259,7 @@ impl PackageStorage {
         // (currently) justify the additional code complexity. If such situation
         // occur and it will be reported, then we probably would consider a
         // better solution (i.e., file locking or checksums).
-        match fs::rename(&package_download_dir, package_dir) {
+        match fs::rename(&extracted_package_dir, package_dir) {
             Ok(()) => Ok(()),
             Err(err) if err.kind() == std::io::ErrorKind::DirectoryNotEmpty => {
                 removed_download_dir()