diff --git a/crates/typst-cli/src/world.rs b/crates/typst-cli/src/world.rs index 42083ef20..f44dd6c65 100644 --- a/crates/typst-cli/src/world.rs +++ b/crates/typst-cli/src/world.rs @@ -404,7 +404,9 @@ fn system_path( // Join the path to the root. If it tries to escape, deny // access. Note: It can still escape via symlinks. - id.vpath().resolve(root).ok_or(FileError::AccessDenied) + id.vpath() + .resolve(root) + .ok_or_else(|| FileError::AccessDenied(id.vpath().as_rootless_path().into())) } /// Reads a file from a `FileId`. @@ -427,7 +429,7 @@ fn read( fn read_from_disk(path: &Path) -> FileResult> { let f = |e| FileError::from_io(e, path); if fs::metadata(path).map_err(f)?.is_dir() { - Err(FileError::IsDirectory) + Err(FileError::IsDirectory(path.into())) } else { fs::read(path).map_err(f) } diff --git a/crates/typst-kit/src/package.rs b/crates/typst-kit/src/package.rs index 06e9f04d6..6cefd84f8 100644 --- a/crates/typst-kit/src/package.rs +++ b/crates/typst-kit/src/package.rs @@ -87,6 +87,7 @@ impl PackageStorage { ) -> PackageResult { let subdir = format!("{}/{}/{}", spec.namespace, spec.name, spec.version); + // By default, search for the package locally if let Some(packages_dir) = &self.package_path { let dir = packages_dir.join(&subdir); if dir.exists() { @@ -94,6 +95,7 @@ impl PackageStorage { } } + // As a fallback, look into the cache and possibly download from network. if let Some(cache_dir) = &self.package_cache_path { let dir = cache_dir.join(&subdir); if dir.exists() { @@ -102,14 +104,42 @@ impl PackageStorage { // Download from network if it doesn't exist yet. if spec.namespace == DEFAULT_NAMESPACE { - self.download_package(spec, cache_dir, progress)?; - if dir.exists() { - return Ok(dir); - } + return self.download_package(spec, cache_dir, progress); } } - Err(PackageError::NotFound(spec.clone())) + // None of the strategies above found the package, so all code paths + // from now on fail. The rest of the function is only to determine the + // cause of the failure. + // We try `namespace/` then `namespace/name/` then `namespace/name/version/` + // and see where the first error occurs. + let not_found = |msg| Err(PackageError::NotFound(spec.clone(), msg)); + + let Some(packages_dir) = &self.package_path else { + return not_found(eco_format!("cannot access local package storage")); + }; + let namespace_dir = packages_dir.join(format!("{}", spec.namespace)); + if !namespace_dir.exists() { + return not_found(eco_format!( + "namespace @{} should be located at {}", + spec.namespace, + namespace_dir.display() + )); + } + let package_dir = namespace_dir.join(format!("{}", spec.name)); + if !package_dir.exists() { + return not_found(eco_format!( + "{} does not have package '{}'", + namespace_dir.display(), + spec.name + )); + } + let latest = self.determine_latest_version(&spec.versionless()).ok(); + Err(PackageError::VersionNotFound( + spec.clone(), + latest, + eco_format!("{}", namespace_dir.display()), + )) } /// Tries to determine the latest version of a package. @@ -171,21 +201,29 @@ impl PackageStorage { spec: &PackageSpec, cache_dir: &Path, progress: &mut dyn Progress, - ) -> PackageResult<()> { + ) -> PackageResult { assert_eq!(spec.namespace, DEFAULT_NAMESPACE); - let url = format!( - "{DEFAULT_REGISTRY}/{DEFAULT_NAMESPACE}/{}-{}.tar.gz", - spec.name, spec.version - ); + let namespace_url = format!("{DEFAULT_REGISTRY}/{DEFAULT_NAMESPACE}"); + let url = format!("{namespace_url}/{}-{}.tar.gz", spec.name, spec.version); let data = match self.downloader.download_with_progress(&url, progress) { Ok(data) => data, Err(ureq::Error::Status(404, _)) => { if let Ok(version) = self.determine_latest_version(&spec.versionless()) { - return Err(PackageError::VersionNotFound(spec.clone(), version)); + return Err(PackageError::VersionNotFound( + spec.clone(), + Some(version), + eco_format!("{namespace_url}"), + )); } else { - return Err(PackageError::NotFound(spec.clone())); + return Err(PackageError::NotFound( + spec.clone(), + eco_format!( + "{namespace_url} does not have package '{}'", + spec.name + ), + )); } } Err(err) => { @@ -235,8 +273,8 @@ impl PackageStorage { // broken packages still occur even with the rename safeguard, we might // consider more complex solutions like file locking or checksums. match fs::rename(&tempdir, &package_dir) { - Ok(()) => Ok(()), - Err(err) if err.kind() == io::ErrorKind::DirectoryNotEmpty => Ok(()), + Ok(()) => Ok(package_dir), + Err(err) if err.kind() == io::ErrorKind::DirectoryNotEmpty => Ok(package_dir), Err(err) => Err(error("failed to move downloaded package directory", err)), } } diff --git a/crates/typst-library/src/diag.rs b/crates/typst-library/src/diag.rs index 2b9ff6376..b7256842b 100644 --- a/crates/typst-library/src/diag.rs +++ b/crates/typst-library/src/diag.rs @@ -439,13 +439,13 @@ pub enum FileError { /// A file was not found at this path. NotFound(PathBuf), /// A file could not be accessed. - AccessDenied, + AccessDenied(PathBuf), /// A directory was found, but a file was expected. - IsDirectory, + IsDirectory(PathBuf), /// The file is not a Typst source file, but should have been. - NotSource, + NotSource(PathBuf), /// The file was not valid UTF-8, but should have been. - InvalidUtf8, + InvalidUtf8(Option), /// The package the file is part of could not be loaded. Package(PackageError), /// Another error. @@ -459,11 +459,11 @@ impl FileError { pub fn from_io(err: io::Error, path: &Path) -> Self { match err.kind() { io::ErrorKind::NotFound => Self::NotFound(path.into()), - io::ErrorKind::PermissionDenied => Self::AccessDenied, + io::ErrorKind::PermissionDenied => Self::AccessDenied(path.into()), io::ErrorKind::InvalidData if err.to_string().contains("stream did not contain valid UTF-8") => { - Self::InvalidUtf8 + Self::InvalidUtf8(Some(path.into())) } _ => Self::Other(Some(eco_format!("{err}"))), } @@ -478,10 +478,19 @@ impl Display for FileError { Self::NotFound(path) => { write!(f, "file not found (searched at {})", path.display()) } - Self::AccessDenied => f.pad("failed to load file (access denied)"), - Self::IsDirectory => f.pad("failed to load file (is a directory)"), - Self::NotSource => f.pad("not a typst source file"), - Self::InvalidUtf8 => f.pad("file is not valid utf-8"), + Self::AccessDenied(path) => { + write!(f, "failed to load file {} (access denied)", path.display()) + } + Self::IsDirectory(path) => { + write!(f, "failed to load file {} (is a directory)", path.display()) + } + Self::NotSource(path) => { + write!(f, "{} is not a typst source file", path.display()) + } + Self::InvalidUtf8(Some(path)) => { + write!(f, "file {} is not valid utf-8", path.display()) + } + Self::InvalidUtf8(None) => f.pad("file is not valid utf-8"), Self::Package(error) => error.fmt(f), Self::Other(Some(err)) => write!(f, "failed to load file ({err})"), Self::Other(None) => f.pad("failed to load file"), @@ -491,13 +500,13 @@ impl Display for FileError { impl From for FileError { fn from(_: Utf8Error) -> Self { - Self::InvalidUtf8 + Self::InvalidUtf8(None) } } impl From for FileError { fn from(_: FromUtf8Error) -> Self { - Self::InvalidUtf8 + Self::InvalidUtf8(None) } } @@ -518,13 +527,15 @@ pub type PackageResult = Result; /// An error that occurred while trying to load a package. /// -/// Some variants have an optional string can give more details, if available. +/// Some variants have an optional string that can give more details, if available. #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub enum PackageError { /// The specified package does not exist. - NotFound(PackageSpec), + /// Additionally provides information on where we tried to find the package. + NotFound(PackageSpec, EcoString), /// The specified package found, but the version does not exist. - VersionNotFound(PackageSpec, PackageVersion), + /// TODO: make the registry part of the error better typed + VersionNotFound(PackageSpec, Option, EcoString), /// Failed to retrieve the package through the network. NetworkFailed(Option), /// The package archive was malformed. @@ -538,15 +549,20 @@ impl std::error::Error for PackageError {} impl Display for PackageError { fn fmt(&self, f: &mut Formatter) -> fmt::Result { match self { - Self::NotFound(spec) => { - write!(f, "package not found (searched for {spec})",) + Self::NotFound(spec, detail) => { + write!(f, "package not found: {detail} (searching for {spec})",) } - Self::VersionNotFound(spec, latest) => { + Self::VersionNotFound(spec, latest, registry) => { write!( f, - "package found, but version {} does not exist (latest is {})", - spec.version, latest, - ) + "package '{}' found, but version {} does not exist", + spec.name, spec.version + )?; + if let Some(version) = latest { + write!(f, " (latest version provided by {registry} is {version})") + } else { + write!(f, " ({registry} contains no versions for this package)") + } } Self::NetworkFailed(Some(err)) => { write!(f, "failed to download package ({err})") diff --git a/crates/typst/src/lib.rs b/crates/typst/src/lib.rs index 47e5531fd..a43065085 100644 --- a/crates/typst/src/lib.rs +++ b/crates/typst/src/lib.rs @@ -191,7 +191,7 @@ fn hint_invalid_main_file( file_error: FileError, input: FileId, ) -> EcoVec { - let is_utf8_error = matches!(file_error, FileError::InvalidUtf8); + let is_utf8_error = matches!(file_error, FileError::InvalidUtf8(_)); let mut diagnostic = SourceDiagnostic::error(Span::detached(), EcoString::from(file_error)); diff --git a/tests/src/world.rs b/tests/src/world.rs index 85969700d..8d4e9c71f 100644 --- a/tests/src/world.rs +++ b/tests/src/world.rs @@ -172,7 +172,9 @@ pub(crate) fn system_path(id: FileId) -> FileResult { None => PathBuf::new(), }; - id.vpath().resolve(&root).ok_or(FileError::AccessDenied) + id.vpath() + .resolve(&root) + .ok_or_else(|| FileError::AccessDenied(id.vpath().as_rootless_path().into())) } /// Read a file. @@ -186,7 +188,7 @@ pub(crate) fn read(path: &Path) -> FileResult> { let f = |e| FileError::from_io(e, path); if fs::metadata(path).map_err(f)?.is_dir() { - Err(FileError::IsDirectory) + Err(FileError::IsDirectory(path.into())) } else { fs::read(path).map(Cow::Owned).map_err(f) } diff --git a/tests/suite/scripting/import.typ b/tests/suite/scripting/import.typ index 382e444cc..f094042bc 100644 --- a/tests/suite/scripting/import.typ +++ b/tests/suite/scripting/import.typ @@ -302,11 +302,11 @@ #import 5 as x --- import-from-string-invalid --- -// Error: 9-11 failed to load file (is a directory) +// Error: 9-11 failed to load file tests/suite/scripting (is a directory) #import "": name --- import-from-string-renamed-invalid --- -// Error: 9-11 failed to load file (is a directory) +// Error: 9-11 failed to load file tests/suite/scripting (is a directory) #import "" as x --- import-file-not-found-invalid --- @@ -483,3 +483,4 @@ This is never reached. --- import-from-file-package-lookalike --- // Error: 9-28 file not found (searched at tests/suite/scripting/#test/mypkg:1.0.0) #import "#test/mypkg:1.0.0": * +