diff --git a/Cargo.toml b/Cargo.toml index d1cbd22..2849095 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "typst-package-check" version = "0.4.6" -edition = "2021" +edition = "2024" [dependencies] casbab = "0.1.1" diff --git a/src/action.rs b/src/action.rs index b4d5606..8eb1213 100644 --- a/src/action.rs +++ b/src/action.rs @@ -1,8 +1,9 @@ use crate::{ check::TryExt, github::{ - api::{pr::PullRequestEvent, GitHubAuth, Installation, Repository}, - run_github_check, AppState, + AppState, + api::{GitHubAuth, Installation, Repository, pr::PullRequestEvent}, + run_github_check, }, }; diff --git a/src/check.rs b/src/check.rs index 72b414a..19727e1 100644 --- a/src/check.rs +++ b/src/check.rs @@ -2,8 +2,8 @@ use std::path::PathBuf; use codespan_reporting::diagnostic::Label; use typst::{ - syntax::{package::PackageSpec, FileId, Span}, WorldExt, + syntax::{FileId, Span, package::PackageSpec}, }; use crate::world::SystemWorld; @@ -19,6 +19,7 @@ mod path; mod readme; pub use diagnostics::{Diagnostics, Result, TryExt}; +pub use manifest::Exclude; pub async fn all_checks( package_spec: Option<&PackageSpec>, diff --git a/src/check/authors.rs b/src/check/authors.rs index b0a7fb0..5a4eaae 100644 --- a/src/check/authors.rs +++ b/src/check/authors.rs @@ -1,5 +1,5 @@ use codespan_reporting::diagnostic::{Diagnostic, Label}; -use typst::syntax::{package::PackageSpec, FileId, VirtualPath}; +use typst::syntax::{FileId, VirtualPath, package::PackageSpec}; use crate::{github::git, package::PackageExt}; diff --git a/src/check/compile.rs b/src/check/compile.rs index a986139..3482a42 100644 --- a/src/check/compile.rs +++ b/src/check/compile.rs @@ -7,7 +7,7 @@ use typst::{ use crate::world::SystemWorld; -use super::{label, Diagnostics}; +use super::{Diagnostics, label}; pub fn check(diags: &mut Diagnostics, world: &SystemWorld) -> Option { let result = typst::compile(world); diff --git a/src/check/files.rs b/src/check/files.rs index 6d218d7..55671fe 100644 --- a/src/check/files.rs +++ b/src/check/files.rs @@ -1,12 +1,30 @@ use std::collections::HashSet; +use std::ffi::OsStr; use std::path::Path; use codespan_reporting::diagnostic::{Diagnostic, Label}; +use walkdir::WalkDir; +use crate::check::Diagnostics; use crate::check::manifest::Manifest; use crate::check::path::PackagePath; use crate::check::readme::Readme; -use crate::check::Diagnostics; + +/// Creates a directory iterator with the same settings as the package bundler. +pub fn walk(dir: &Path) -> ignore::Walk { + ignore::WalkBuilder::new(dir) + .sort_by_file_name(|a, b| a.cmp(b)) + // Disable non-local ignore features + .parents(false) + .require_git(false) + .git_global(false) + .git_exclude(false) + // Keep local ignore features for now. + .git_ignore(true) + .ignore(true) + .hidden(true) + .build() +} pub fn check( diags: &mut Diagnostics, @@ -15,37 +33,31 @@ pub fn check( readme: &Option, ) { let exclude = &manifest.package.exclude; - let thumbnail_path = manifest.thumbnail(); - // Manually keep track of excluded directories, to figure out if nested - // files are ignored. This is done, so we can generate diagnostics for - // excluded files. - let mut excluded_dirs = HashSet::new(); + // The bundler enables some local ignore features, which can be confusing. + // Collect the list of files the bundler would consider bundling, without + // the exclude globs. Use it to determine if a file is ignored. + let without_ignored = walk(package_dir) + .flatten() + .filter_map(|ch| { + let metadata = ch.metadata().ok()?; + metadata.is_file().then_some(ch.into_path()) + }) + .collect::>(); - for ch in ignore::WalkBuilder::new(package_dir).hidden(false).build() { - let Ok(ch) = ch else { continue }; + for ch in WalkDir::new(package_dir).into_iter().flatten() { let Ok(metadata) = ch.metadata() else { continue; }; - - let file_path = PackagePath::from_full(package_dir, ch.path()); - - if metadata.is_dir() { - // If the parent directory is ignored, all children are ignored too. - if parent_is_excluded(&excluded_dirs, file_path) - || exclude.matched(file_path.relative(), true).is_ignore() - { - excluded_dirs.insert(ch.into_path()); - } + if !metadata.is_file() { continue; } - // The thumbnail is always excluded. - let is_thumbnail = thumbnail_path.is_some_and(|t| t.val == file_path); - let excluded = is_thumbnail - || parent_is_excluded(&excluded_dirs, file_path) - || exclude.matched(file_path.relative(), false).is_ignore(); + let file_path = PackagePath::from_full(package_dir, ch.path()); + let excluded = exclude.matches_file(&file_path); + let ignored = !without_ignored.contains(file_path.full()); + warn_ignored_files(diags, file_path, excluded, ignored); forbid_font_files(diags, file_path); exclude_large_files(diags, file_path, excluded, metadata.len()); exclude_examples_and_tests(diags, file_path, excluded); @@ -53,14 +65,53 @@ pub fn check( } } -fn parent_is_excluded( - excluded_dirs: &HashSet, +fn warn_ignored_files( + diags: &mut Diagnostics, file_path: PackagePath<&Path>, -) -> bool { - file_path - .full() - .parent() - .is_some_and(|parent| excluded_dirs.contains(parent)) + excluded: bool, + ignored: bool, +) { + if excluded || !ignored { + return; + } + + // Don't emit noisy warnings for common hidden files that won't be a problem + // when missing from the bundle. + const COMMON: [&str; 5] = [ + ".gitattributes", + ".gitignore", + ".gitkeep", + ".ignore", + ".keep", + ]; + if COMMON.map(OsStr::new).contains(&file_path.file_name()) { + return; + } + + let (reason, hint) = if file_path.file_name().as_encoded_bytes().starts_with(b".") { + ( + ".\nIt's ignored, because it is hidden: the file name starts with a `.`.", + "If not, consider removing the file.", + ) + } else { + ( + " because of an ignore file, such as `.gitignore` or `.ignore`.", + "If not, consider removing it or updating the ignore file.", + ) + }; + + diags.emit( + Diagnostic::warning() + .with_code("files/ignored") + .with_label(Label::primary(file_path.file_id(), 0..0)) + .with_message(format_args!( + "This file won't be present in the bundled package{reason}\n\n\ + If this is intentional and the file is used for documentation and linked in the readme, \ + consider explicitly adding it to the `exclude` list.\n\ + {hint}\n\n\ + More details: https://github.com/typst/packages/blob/main/docs/tips.md#what-to-commit-what-to-exclude", + )), + ); } fn exclude_large_files( @@ -218,13 +269,15 @@ fn link_manuals( let note = (!excluded) .then(|| "It should also be added to `exclude` in your `typst.toml`.".into()); - diags.emit(Diagnostic::warning() - .with_label(Label::primary(path.file_id(), 0..0)) - .with_code("files/manual/unlinked") - .with_message( - "This file seems to be a manual/documentation, but isn't linked in the readme. \ + diags.emit( + Diagnostic::warning() + .with_label(Label::primary(path.file_id(), 0..0)) + .with_code("files/manual/unlinked") + .with_message( + "This file seems to be a manual/documentation, but isn't linked in the readme. \ It will be inacessible on Typst Universe.", - ) - .with_notes_iter(note)); + ) + .with_notes_iter(note), + ); } } diff --git a/src/check/imports.rs b/src/check/imports.rs index 0df5b80..17e975f 100644 --- a/src/check/imports.rs +++ b/src/check/imports.rs @@ -5,16 +5,16 @@ use std::{ use codespan_reporting::diagnostic::Diagnostic; use typst::{ + World, syntax::{ ast::{self, AstNode, ModuleImport}, package::{PackageSpec, PackageVersion, VersionlessPackageSpec}, }, - World, }; use walkdir::WalkDir; use crate::check::path::PackagePath; -use crate::check::{label, Diagnostics, Result, TryExt}; +use crate::check::{Diagnostics, Result, TryExt, label}; use crate::world::SystemWorld; pub fn check(diags: &mut Diagnostics, package_dir: &Path, world: &SystemWorld) -> Result<()> { @@ -85,23 +85,18 @@ pub fn check_ast( ) } - if let Some(all_packages) = all_packages { - if let Ok(import_spec) = PackageSpec::from_str(source_str.get().as_str()) { - if let Some(latest_version) = - latest_package_version(all_packages, import_spec.versionless()) - { - if latest_version != import_spec.version { - diags.emit( - Diagnostic::warning() - .with_labels(label(world, import.span()).into_iter().collect()) - .with_code("import/outdated") - .with_message( - "This import seems to use an older version of the package.", - ), - ) - } - } - } + if let Some(all_packages) = all_packages + && let Ok(import_spec) = PackageSpec::from_str(source_str.get().as_str()) + && let Some(latest_version) = + latest_package_version(all_packages, import_spec.versionless()) + && latest_version != import_spec.version + { + diags.emit( + Diagnostic::warning() + .with_labels(label(world, import.span()).into_iter().collect()) + .with_code("import/outdated") + .with_message("This import seems to use an older version of the package."), + ) } } } diff --git a/src/check/kebab_case.rs b/src/check/kebab_case.rs index f44f905..af7a337 100644 --- a/src/check/kebab_case.rs +++ b/src/check/kebab_case.rs @@ -3,17 +3,17 @@ use std::collections::HashSet; use codespan_reporting::diagnostic::{Diagnostic, Severity}; use comemo::Track; use typst::{ + ROUTINES, World, engine::{Route, Sink, Traced}, syntax::{ - ast::{self, AstNode}, FileId, Source, SyntaxNode, + ast::{self, AstNode}, }, - World, ROUTINES, }; use crate::world::SystemWorld; -use super::{label, Diagnostics}; +use super::{Diagnostics, label}; // Check that all public identifiers are in kebab-case pub fn check(diags: &mut Diagnostics, world: &SystemWorld) -> Option<()> { diff --git a/src/check/manifest.rs b/src/check/manifest.rs index a148e47..146144c 100644 --- a/src/check/manifest.rs +++ b/src/check/manifest.rs @@ -7,10 +7,11 @@ use reqwest::StatusCode; use toml_edit::{Array, Item, Table}; use tracing::{debug, warn}; use typst::syntax::{ - package::{PackageSpec, PackageVersion}, FileId, VirtualPath, + package::{PackageSpec, PackageVersion}, }; +use crate::check::files; use crate::check::path::{self, PackagePath}; use crate::{ check::{Diagnostics, Result, TryExt}, @@ -26,22 +27,16 @@ pub struct Worlds { #[derive(Debug, Clone)] pub struct Manifest { pub package: Spanned, + #[allow(unused)] pub template: Option>, } -impl Manifest { - pub fn thumbnail(&self) -> Option>> { - let thumbnail = self.template.as_ref()?.thumbnail.as_ref()?; - Some(thumbnail.as_ref().map(PackagePath::as_path)) - } -} - #[derive(Debug, Clone)] pub struct Package { pub entrypoint: Spanned, pub name: Option>, pub version: Option>, - pub exclude: Spanned, + pub exclude: Spanned, } #[derive(Debug, Clone)] @@ -88,7 +83,8 @@ pub async fn check( let entrypoint = entrypoint.map(|e| PackagePath::from_relative(package_dir, e)); let name = check_name(diags, package, package_spec); let version = check_version(diags, package, package_spec); - let exclude = check_exclude(diags, package, package_dir)?; + let template = check_template(diags, &manifest, package_dir); + let exclude = check_exclude(diags, package_dir, package, &template)?; check_compiler_version(diags, package); check_universe_fields(diags, package); @@ -107,9 +103,8 @@ pub async fn check( exclude, }); - let template = check_template(diags, &manifest, package_dir); if let Some(template) = &template { - check_thumbnail(diags, &package.exclude, template); + check_thumbnail(diags, template); dont_exclude_template_files(diags, package_dir, &package.exclude, template); } @@ -177,20 +172,20 @@ fn check_name( ); } - if let Some(package_spec) = package_spec { - if name.val != package_spec.name { - diags.emit( - error - .with_code("manifest/package/name/mismatch") - .with_message(format!( - "Unexpected package name. `{name}` was expected. \ - If you want to publish a new package, create a new \ - directory in `packages/{namespace}/`.", - name = package_spec.name, - namespace = package_spec.namespace, - )), - ) - } + if let Some(package_spec) = package_spec + && name.val != package_spec.name + { + diags.emit( + error + .with_code("manifest/package/name/mismatch") + .with_message(format!( + "Unexpected package name. `{name}` was expected. \ + If you want to publish a new package, create a new \ + directory in `packages/{namespace}/`.", + name = package_spec.name, + namespace = package_spec.namespace, + )), + ) } Some(name.to_owned()) @@ -232,27 +227,27 @@ fn check_version( .with_code("manifest/package/version/invalid") .with_message( "`version` must be a valid semantic version \ - (i.e follow the `MAJOR.MINOR.PATCH` format).", + (i.e follow the `MAJOR.MINOR.PATCH` format).", ), ); return None; }; - if let Some(package_spec) = package_spec { - if version.val != package_spec.version { - diags.emit( - error - .with_code("manifest/package/version/mismatch") - .with_message(format!( - "Unexpected version number. `{version}` was expected. \ - If you want to publish a new version, create a new \ - directory in `packages/{namespace}/{name}`.", - version = package_spec.version, - name = package_spec.name, - namespace = package_spec.namespace, - )), - ) - } + if let Some(package_spec) = package_spec + && version.val != package_spec.version + { + diags.emit( + error + .with_code("manifest/package/version/mismatch") + .with_message(format!( + "Unexpected version number. `{version}` was expected. \ + If you want to publish a new version, create a new \ + directory in `packages/{namespace}/{name}`.", + version = package_spec.version, + name = package_spec.name, + namespace = package_spec.namespace, + )), + ) } Some(version) @@ -284,10 +279,10 @@ fn check_compiler_version(diags: &mut Diagnostics, package: Spanned<&Table>) -> Some(()) } -fn dont_over_exclude(diags: &mut Diagnostics, exclude: &Spanned) -> Result<()> { +fn dont_over_exclude(diags: &mut Diagnostics, exclude: &Spanned) -> Result<()> { let warning = Diagnostic::warning().with_label(Label::primary(manifest_id(), exclude.span())); - if exclude.matched("LICENSE", false).is_ignore() { + if exclude.matches_relative_file("LICENSE") { diags.emit( warning .clone() @@ -296,7 +291,7 @@ fn dont_over_exclude(diags: &mut Diagnostics, exclude: &Spanned) -> Re ); } - if exclude.matched("README.md", false).is_ignore() { + if exclude.matches_relative_file("README.md") { diags.emit( warning .with_code("exclude/readme") @@ -489,7 +484,7 @@ async fn check_repo(diags: &mut Diagnostics, package: Spanned<&Table>) { .with_code("manifest/package/homepage/redundant") .with_message( "Use the homepage field only if there is a dedicated website. \ - Otherwise, prefer the `repository` field.", + Otherwise, prefer the `repository` field.", ), ) } @@ -500,11 +495,12 @@ async fn check_repo(diags: &mut Diagnostics, package: Spanned<&Table>) { /// a lot of false positives in other diagnostics. fn check_exclude( diags: &mut Diagnostics, - package: Spanned<&Table>, package_dir: &Path, -) -> Result> { + package: Spanned<&Table>, + template: &Option>, +) -> Result> { let Some(exclude) = package.get_spanned("exclude") else { - return Ok(Spanned::new(Override::empty(), package.span())); + return Ok(Spanned::new(Exclude::empty(), package.span())); }; let exclude = exclude.try_map(Item::as_array).error( @@ -537,10 +533,74 @@ fn check_exclude( exclude_globs.add(&format!("!{exclusion_str}")).ok(); } + let thumbnail_path = template.as_ref().and_then(|t| t.thumbnail.as_ref()); + if let Some(thumbnail_path) = thumbnail_path { + // Check if the thumbnail is directly excluded, it's fine if a parent + // directory is excluded. + if let Ok(exclude) = exclude_globs.build() + && exclude.matched(thumbnail_path.full(), false).is_ignore() + { + diags.emit( + Diagnostic::error() + .with_label(Label::primary(manifest_id(), thumbnail_path.span())) + .with_code("manifest/template/thumbnail/exclude") + .with_message("The template thumbnail is automatically excluded"), + ); + } + + exclude_globs + .add(&format!("!{}", thumbnail_path.relative().display())) + .ok(); + } + let exclude_globs = exclude_globs .build() .error("manifest/package/exclude/invalid", "Invalid exclude globs")?; - Ok(exclude.map(|_| exclude_globs)) + Ok(exclude.map(|_| Exclude::new(exclude_globs))) +} + +#[derive(Debug, Clone)] +pub struct Exclude { + globs: Override, +} + +impl Exclude { + pub fn empty() -> Self { + Self { + globs: Override::empty(), + } + } + + pub fn new(globs: Override) -> Self { + Self { globs } + } + + /// Whether the package file path is excluded. + pub fn matches_file>(&self, path: &PackagePath) -> bool { + self.matches_relative_file(path.relative()) + } + + /// Whether the package relative file path is excluded. + pub fn matches_relative_file(&self, relative_path: impl AsRef) -> bool { + let relative_path = relative_path.as_ref(); + if self.globs.matched(relative_path, false).is_ignore() { + return true; + } + + // Manually check if any of the parent directories is excluded. + // This has to be done to mimic the behavior of [`ignore::Walk`], which + // won't enter a directory if it is ignored. [`Override::matched] + // function doesn't check if a parent directory is excluded. + let mut parent = relative_path.parent(); + while let Some(dir) = parent { + if self.globs.matched(dir, true).is_ignore() { + return true; + } + parent = dir.parent(); + } + + false + } } fn check_template( @@ -569,7 +629,7 @@ fn check_template( let template_dir = path.as_ref()?; Some( entrypoint - .map(|entrypoint| path::relative_to(template_dir.full(), entrypoint)) + .map(|entrypoint| path::join_to(template_dir.full(), entrypoint)) .map(|path| PackagePath::from_full(package_dir, path)), ) }); @@ -618,14 +678,14 @@ fn world_for_template( fn dont_exclude_template_files( diags: &mut Diagnostics, package_dir: &Path, - exclude: &Override, + exclude: &Exclude, template: &Spanned