Merge pull request #22 from GuillaumeGomez/rename-from

Add new rename-from field to ensure checks when source URL file differs from `name`

Author: Kobzol

README.md

@@ -31,6 +31,10 @@ the TOML files in the `files/` directory. Each entry has the following schema:
   artifacts built from open source code you should put the license identifier,
   for everything else you should put a link to the licensing terms.
 
+* **`rename-from`**: in case the `source` file has a different name than `name`,
+  you need to add this field to explicitly mark that this is expected with the
+  file name from `source`.
+
 You can add a new entry either by manually modifying a TOML file in the `files` directory,
 or by using the following command:
 

files/gha-self-hosted.toml

@@ -3,3 +3,4 @@ name = "gha-self-hosted/qemu-efi-aarch64_2022.11-6.deb"
 source = "http://ftp.debian.org/debian/pool/main/e/edk2/qemu-efi-aarch64_2022.11-6+deb12u2_all.deb"
 sha256 = "9a55c7b94fdf13a28928359a77d42e5364aa3ae2e558bd1fd5361955bf479d81"
 license = "BSD"
+rename-from = "qemu-efi-aarch64_2022.11-6+deb12u2_all.deb"

src/main.rs

@@ -167,11 +167,21 @@ async fn add_file(args: AddFileArgs) -> anyhow::Result<()> {
         .append(true)
         .open(&args.toml_file)?;
 
+    let rename_from = if let Some(file_name) = args.url.path().split('/').last()
+        && let Some(path_name) = args.path.split('/').last()
+        && file_name != path_name
+    {
+        Some(file_name.to_string())
+    } else {
+        None
+    };
+
     let entry = ManifestFileManaged::new(
         args.path,
         to_hex(&hash),
         args.url,
         args.license.unwrap_or(String::new()),
+        rename_from,
     );
     let entry = toml::to_string(&entry)?;
 

src/manifest.rs

@@ -44,44 +44,118 @@ struct LocationCache {
 pub(crate) fn load_manifests(load_from: &Path) -> Result<(Vec<MirrorFile>, Vec<String>), Error> {
     let mut result = Vec::new();
     let mut cache = LocationCache::default();
+    let mut errors = Vec::new();
+
+    fn emit_error(
+        error: String,
+        mirror_file: &MirrorFile,
+        file_source: &str,
+        cache: &LocationCache,
+        errors: &mut Vec<String>,
+    ) {
+        let location = cache
+            .seen_paths
+            .get(&mirror_file.name)
+            .unwrap()
+            .first()
+            .unwrap();
+        let (src_line, snippet) = span_info(&file_source, location);
+        errors.push(format!(
+            "{error}:\n\
+             # {} (line {src_line})\n{snippet}\n",
+            location.file.display()
+        ));
+    }
 
     fn load_inner(
         load_from: &Path,
         result: &mut Vec<MirrorFile>,
         cache: &mut LocationCache,
+        errors: &mut Vec<String>,
     ) -> anyhow::Result<()> {
         for entry in load_from.read_dir()? {
             let path = entry?.path();
             if path.is_file() && path.extension().and_then(|s| s.to_str()) == Some("toml") {
-                let manifest = std::fs::read_to_string(&path)
+                let file_source = std::fs::read_to_string(&path)
+                    .map_err(Error::from)
+                    .with_context(|| format!("failed to read {}", path.display()))?;
+                let manifest = toml::from_str::<Manifest>(&file_source)
                     .map_err(Error::from)
-                    .and_then(|raw| toml::from_str::<Manifest>(&raw).map_err(Error::from))
                     .with_context(|| format!("failed to read {}", path.display()))?;
                 record_locations(&path, &manifest, cache);
 
                 for file in manifest.files {
-                    result.push(match file.into_inner() {
+                    let mirror_file = match file.into_inner() {
                         ManifestFile::Legacy(legacy) => MirrorFile {
                             name: legacy.name,
                             sha256: legacy.sha256,
                             source: Source::Legacy,
+                            rename_from: None,
                         },
                         ManifestFile::Managed(managed) => MirrorFile {
                             name: managed.name,
                             sha256: managed.sha256,
                             source: Source::Url(managed.source),
+                            rename_from: managed.rename_from,
                         },
-                    });
+                    };
+                    if let Source::Url(ref source) = mirror_file.source {
+                        if let Some(file_name) = source.path().split('/').last()
+                            && let Some(path_name) = mirror_file.name.split('/').last()
+                        {
+                            match mirror_file.rename_from {
+                                Some(ref rename_from) => {
+                                    if path_name == file_name {
+                                        emit_error(
+                                            format!(
+                                                "`rename-from` field isn't needed since `source` and `name` field have the same file name (`{file_name}`)"
+                                            ),
+                                            &mirror_file,
+                                            &file_source,
+                                            cache,
+                                            errors,
+                                        );
+                                    } else if rename_from != file_name {
+                                        emit_error(
+                                            format!(
+                                                "`rename-from` field value doesn't match name from the URL `{source}` (`{file_name}` != `{rename_from}`)"
+                                            ),
+                                            &mirror_file,
+                                            &file_source,
+                                            cache,
+                                            errors,
+                                        );
+                                    }
+                                }
+                                None => {
+                                    if path_name != file_name {
+                                        emit_error(
+                                            format!(
+                                                "The name from the URL `{source}` doesn't match the `name` field (`{file_name}` != `{path_name}`). \
+                                             Add `rename-from = {file_name:?}` to fix this error"
+                                            ),
+                                            &mirror_file,
+                                            &file_source,
+                                            cache,
+                                            errors,
+                                        );
+                                    }
+                                }
+                            }
+                        }
+                    }
+                    result.push(mirror_file);
                 }
             } else if path.is_dir() {
-                load_inner(&path, result, cache)?;
+                load_inner(&path, result, cache, errors)?;
             }
         }
         Ok(())
     }
 
-    load_inner(load_from, &mut result, &mut cache)?;
-    Ok((result, find_errors(cache)))
+    load_inner(load_from, &mut result, &mut cache, &mut errors)?;
+    find_errors(cache, &mut errors);
+    Ok((result, errors))
 }
 
 fn record_locations(toml_path: &Path, manifest: &Manifest, cache: &mut LocationCache) {
@@ -114,12 +188,32 @@ fn record_locations(toml_path: &Path, manifest: &Manifest, cache: &mut LocationC
             .or_default()
             .insert(location.clone());
         if let Some(url) = url {
-            cache.seen_urls.entry(url).or_default().insert(location);
+            cache
+                .seen_urls
+                .entry(url)
+                .or_default()
+                .insert(location.clone());
         }
     }
 }
 
-fn find_errors(cache: LocationCache) -> Vec<String> {
+fn span_info<'a>(content: &'a str, location: &Location) -> (usize, &'a str) {
+    // Find the corresponding line number
+    let mut accumulated_chars = 0;
+    let mut src_line = 0;
+    for (index, line) in content.lines().enumerate() {
+        accumulated_chars += line.len() + 1; // +1 for newline
+        if accumulated_chars > location.span.0.start {
+            src_line = index + 1;
+            break;
+        }
+    }
+
+    let snippet = &content[location.span.0.start..location.span.0.end];
+    (src_line, snippet)
+}
+
+fn find_errors(cache: LocationCache, errors: &mut Vec<String>) {
     let mut file_cache: HashMap<PathBuf, String> = HashMap::new();
 
     fn format_locations(
@@ -136,18 +230,7 @@ fn find_errors(cache: LocationCache) -> Vec<String> {
                 })
             });
 
-            // Find the corresponding line number
-            let mut accumulated_chars = 0;
-            let mut src_line = 0;
-            for (index, line) in content.lines().enumerate() {
-                accumulated_chars += line.len() + 1; // +1 for newline
-                if accumulated_chars > location.span.0.start {
-                    src_line = index + 1;
-                    break;
-                }
-            }
-
-            let snippet = &content[location.span.0.start..location.span.0.end];
+            let (src_line, snippet) = span_info(&content, location);
             writeln!(
                 output,
                 "# {} (line {src_line})\n{snippet}\n",
@@ -159,7 +242,6 @@ fn find_errors(cache: LocationCache) -> Vec<String> {
         output
     }
 
-    let mut errors = Vec::new();
     for (path, locations) in cache.seen_paths {
         if locations.len() > 1 {
             errors.push(format!(
@@ -184,13 +266,13 @@ fn find_errors(cache: LocationCache) -> Vec<String> {
             ));
         }
     }
-    errors
 }
 
 pub(crate) struct MirrorFile {
     pub(crate) name: String,
     pub(crate) sha256: String,
     pub(crate) source: Source,
+    pub(crate) rename_from: Option<String>,
 }
 
 pub(crate) enum Source {
@@ -233,15 +315,24 @@ pub struct ManifestFileManaged {
     // This field is not considered at all by the automation, we just enforce its presence so that
     // people adding new entries think about the licensing implications.
     license: String,
+    #[serde(default, rename = "rename-from")]
+    rename_from: Option<String>,
 }
 
 impl ManifestFileManaged {
-    pub fn new(name: String, sha256: String, source: Url, license: String) -> Self {
+    pub fn new(
+        name: String,
+        sha256: String,
+        source: Url,
+        license: String,
+        rename_from: Option<String>,
+    ) -> Self {
         Self {
             name,
             sha256,
             source,
             license,
+            rename_from,
         }
     }
 }