diff --git a/crates/typst-kit/src/package.rs b/crates/typst-kit/src/package.rs index ce6d374ac..6f388bf8c 100644 --- a/crates/typst-kit/src/package.rs +++ b/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 { 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)) = - (&self.package_cache_path, download_dir) - { + if let Some(cache_dir) = &self.package_cache_path { 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 - // to the target directory; - let package_download_dir = download_dir.join(format!( - "{}-{}-{}", + // Temporary place where the package will be extracted before being + // moved to the target directory. + // + // 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::() // Make directory name unique. + rand::random::() )); 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 - // operation without the help of file locking. + // concurrently, we are abusing the behavior of the `rename` FS operation + // 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` - // are on the same mount point. + // By extracting package into the same base directory where all packages + // 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()