~science-computing/butido: Fix: Patch file finding algorithm

f78df9eeb3c8d6aa6f74c0bb774282662b1ef635 — Matthias Beyer 3 years ago 8bf7702

Fix: Patch file finding algorithm

Because commit 5c36c119f9448baf6bfe5245c6ebac1aa09d5b43 was not enough and was
actually buggy, this fixes the patch-file-finding algorithm _again_.

The approach changed a bit. It actually introduces even more overhead to the
loading algorithm, because of constant type-conversions. But it's as good as it
can be right now.

So first of all, we're collecting the patches before the merge into a
Vec<PathBuf>. Each of those are existing.

Then we check whether the new patch exists, and if it not does, we check whether
the file actually exists in the patches from before the merge (by filename). If
it does, it seems that we dragged the entry from the previous recursion. In
this case, the patches from before the merge were valid, and the recursion
invalidated the path.

I.E.:

/pkg.toml with patches = [ "a" ]
/a
/sub/pkg.toml with no patches=[]

in the recursion for /sub/pkg.toml, we get a ["a"] in the patches array, because
that's how the layered configuration works.
But because the path is invalid, we check whether before the merge (so, the
patches array from /pkg.toml) has a file named equally - which it does. So the
array before the merge is the correct one.

I did some tests on this and it seems to work correctly, but more edge-cases may
exist.

Signed-off-by: Matthias Beyer <matthias.beyer@atos.net>
Tested-by: Matthias Beyer <matthias.beyer@atos.net>
Fixes: 5c36c119f9448baf6bfe5245c6ebac1aa09d5b43 ("Fix: Error out if a patch file is missing")

patch browse .tar.gz

1 files changed, 25 insertions(+), 3 deletions(-)

M src/repository/repository.rs

M src/repository/repository.rs => src/repository/repository.rs +25 -3

@@ 18,6 18,7 @@ use anyhow::Error;
 use anyhow::Result;
 use log::trace;
 use resiter::AndThen;
+use resiter::FilterMap;
 use resiter::Map;
 
 use crate::package::Package;


@@ 107,7 108,16 @@ impl Repository {
                 // This is either the patches array from the last recursion or the newly set one,
                 // that doesn't matter here.
                 let patches_before_merge = match config.get_array("patches") {
-                    Ok(v)                                 => v,
+                    Ok(v)  => {
+                        v.into_iter()
+                            .map(|p| {
+                                p.into_str()
+                                    .map(PathBuf::from)
+                                    .with_context(|| anyhow!("patches must be strings"))
+                                    .map_err(Error::from)
+                            })
+                            .collect::<Result<Vec<_>>>()?
+                    },
                     Err(config::ConfigError::NotFound(_)) => vec![],
                     Err(e)                                => return Err(e).map_err(Error::from),
                 };


@@ 145,22 155,34 @@ impl Repository {
                 // Otherwise we have an error here, because we're refering to a non-existing file.
                 .and_then_ok(|patch| if patch.exists() {
                     trace!("Path to patch exists: {}", patch.display());
-                    Ok(config::Value::from(patch.display().to_string()))
+                    Ok(Some(patch))
+                } else if patches_before_merge.iter().any(|pb| pb.file_name() == patch.file_name()) {
+                    // We have a patch already in the array that is named equal to the patch
+                    // we have in the current recursion.
+                    // It seems like this patch was already in the list and we re-found it
+                    // because we loaded a deeper pkg.toml file.
+                    Ok(None)
                 } else {
                     trace!("Path to patch does not exist: {}", patch.display());
                     Err(anyhow!("Patch does not exist: {}", patch.display()))
                 })
+                .filter_map_ok(|o| o)
                 .collect::<Result<Vec<_>>>()?;
 
                 // If we found any patches, use them. Otherwise use the array from before the merge
                 // (which already has the correct pathes from the previous recursion).
-                let patches = if !patches.is_empty() {
+                let patches = if !patches.is_empty() && patches.iter().all(|p| p.exists()) {
                     patches
                 } else {
                     patches_before_merge
                 };
 
                 trace!("Patches after postprocessing merge: {:?}", patches);
+                let patches = patches
+                    .into_iter()
+                    .map(|p| p.display().to_string())
+                    .map(config::Value::from)
+                    .collect::<Vec<_>>();
                 config.set_once("patches", config::Value::from(patches))?;
             }