File deletion masks are incorrectly collected into an spk package

[jrray]

The setup for this is complicated (hopefully a test can be created to exercise this behavior).

We have a build of python-pip that was created using spk convert pip -- pip, and when doing this specific conversion of pip, the recipe has this validation rule in place:

build:
  validation:
    rules:
    - allow: RecursiveBuild

The resulting package's manifest ends up with a mask entry like so:

020000 mask AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA==== /spfs/lib/python3.9/site-packages/pip-21.2.3.dist-info

Before doing the spk convert pip -- pip, a different build of pip was made "by hand" that utilizes ensurepip and creates version 21.2.3, then this spk convert pip -- pip is creating version 24.0.

Aren't masks supposed to not be captured into spk packages in the first place? I think there might be a bug in the recursive package support.

Then, once you have a python-pip package that contains this mask, entering into an spk environment that includes python python-pip (as is what happens when using spk convert pip on other packages) writes a mask file into the upperdir before the build begins:

DEBUG masking deleted files...
TRACE build parent dir for mask parent="tmp/spfs-runtime/upper/lib/python3.9/site-packages"
TRACE Creating file mask node.path="/tmp/spfs-runtime/upper/lib/python3.9/site-packages/pip-21.2.3.dist-info"

The secondary problem here is that when the files are being collected after the build that happened in this runtime, e.g., for spk convert pip -- linecache2), this mask is collected into the package as if this package deleted that directory:

DEBUG spk_build::build::binary: python-linecache2:run collecting "/lib/python3.9/site-packages/pip-21.2.3.dist-info"

So this fact that this mask exists inside the python-pip package makes it propagate into all subsequent pip converted packages.

There are two distinct bugs here:

1. the build of python-pip should not have collected a mask when it deleted a directory belonging to the recursive version of itself;
2. any masks put into place during the creation of the runtime should not be collected as masks in the newly created package.

[jrray]

Here's a look at the manifest of the converted package:

Note that the recipe for this package doesn't enable any custom validation rules and the fact this mask is present at collection time does not trigger any validation violations. I guess technically this path is not owned by any of the packages in the dependency tree, though the mask was inherited from one of the dependencies.

[dcookspi]

It looks like the capturing of DiffMode::Removed changes was changed in the validation rework.

Assuming I'm reading things corectly, in the old crates/spk-schema/crates/validators/src/validators.rs must_collect_all_files function (from commit 95b6a4391be6e0ce3d414992f40b323ddcb69da4), this:

           let entry = match &d.mode {
                spfs::tracking::DiffMode::Unchanged(e) => e,
                spfs::tracking::DiffMode::Changed(_, e) => e,
                spfs::tracking::DiffMode::Added(e) => e,
                spfs::tracking::DiffMode::Removed(_) => return false,
            };

was moved to the build_and_commit_artifacts method in crates/spk-build/src/build/binary.rs and became (in HEAD):

              match diff.mode {
                    // Filter out `DiffMode::Removed` entries that aren't `EntryKind::Mask`.
                    // Since we didn't provide the complete manifest for all of /spfs, but
                    // just for the overlayfs upperdir instead, anything that wasn't changed
                    // will show up in the diff as removed.
                    DiffMode::Removed(ref e) if e.kind == spfs::tracking::EntryKind::Mask => {
                        Some(diff)
                    }
                    DiffMode::Removed(ref e) =>  None,
                    // Unchanged diffs represent files that were in the working changes/upperdir
                    // but whose contents and permissions were the same as the base layer. In other
                    // words they were touched but not changed. These files are ignored unless
                    // they belong to another version of the package being built. In these cases
                    // we assume that this is a known recursive build, or at least that these files
                    // were written as part of the build but unknowingly clashed with the existing
                    // package. In these cases this type of change would need to be manually reset
                    // during the build script to not be collected.
                    DiffMode::Unchanged(src) if src.user_data.name() != input.package.name() => {
                        None
                    }
                    _ => Some(diff),

So some Removed things are now included, and others are not?

[dcookspi]

For bug 2. the mask is being collected as an Add change for the newly created package.

[jrray]

@dcookspi there are a number of tricky cases to handle recursive packages correctly, see the "circular_dep" tests in cmd_build_test. In particular, test_package_with_circular_dep_does_not_collect_file_removals is intended to test the situation that appears to be going wrong here, where the package build ends up deleting a file (directory in our new case) belonging to the circular dependency.

[rydrman]

At first glance, it feels like a mix between:

1. the validation changed so that we can detect, fail on, but ultimately override file deletions during a build (rather than blanket ignoring them as we used to)
2. the recursive build allows a package to change a previous version of itself (which bypasses the validation for file removals, rather than ignoring the removal)

[jrray]

2. the recursive build allows a package to change a previous version of itself (which bypasses the validation for file removals, rather than ignoring the removal)

Except that the aforementioned test is trying to prove that this isn't true.

[rydrman]

Hmm, right - ya that's a head scratcher 🤔

[dcookspi]

As another data point for bug 2., spfs reset, which spk build does internally doesn't remove the spurious Mask that comes in the from the spk built pip package. This doesn't seem right to me.

It shouldn't matter that the Masked pip-21.2.3.dist-info might've been a dir not a file should it? The pip-24.0.dist-info that replaced it in the newer pip build is a dir, as mentioned. But a Mask should cope with that shouldn't it?

[jrray]

For bug 2. the mask is being collected as an Add change for the newly created package.

As of #1037 the mask won't be collected as an Added anymore, so I'm hoping this fixes bug 2.