common/build-style/python3-pep517.sh: ignore tests in temp destdir

[icp1994]

Testing the changes

• I tested the changes in this PR: YES

An edge case I came across today. On rare cases, packages include tests in their built wheel. While testing with these wheels extracted in a temporary directory, it conflicts with the tests in the source tree as pytest cannot run testfiless with same name in different directories (ref: pytest-dev/pytest#3151).

See this run as an example. The same package passes checks correctly in this PR.

[tornaria]

This PR fixes the first time check is run, but not the second and subsequent runs.

In fact, the wheel will be installed into a different testdir each time (and not removed afterwards) and this only ignores the current one.

Suggestions:

• use testdir=$(mktemp -d) so the testdir is outside the wrksrc tree.
• use --ignore=${wrksrc}/tmp so we ignore all installations.

[icp1994]

Oh, right.

I'll keep it in wrksrc so that it gets auto-cleaned by xbps-src.

[tornaria]

LGTM now. Just FTR this merge confilcts with #44071 but both PR are necessary.

[icp1994]

Yeah I'll rebase once that's merged into master.

[ahesford]

This seems reasonable. Using mktemp to create directories outside of $wrksrc is undesirable because we might potentially leave artifacts that aren't cleaned up by xbps-src clean. Sure, it will be in /tmp, but it's still better to be sure it gets cleaned up after the build.

[ahesford]

Hrm, should we just remove the tmpdir after the tests are run?

[icp1994]

Where would you put the rm call though? If the pytest call fails, it won't get to that.

[ahesford]

That's okay, because failing tests will already abort a build and any subsequent attempt probably ought to start from a clean masterdir for proper behavior anyway. Cleaning up at the end of do_check after a successful test should be good enough. Leaving the temporary directory around after a failed test also lets users inspect the installation for defects that might have triggered the error.

[icp1994]

Oh, okay. I thought the norm was if the build is successful, one should be able to run ./xbps-src check multiple times on failed tests without running ./xbps-src clean

[ahesford]

Sure, it's reasonable to repeatedly run check to work out test issues, but when it comes time to build and install the final package, I definitely recommend a clean build first.

[ahesford]

I've been thinking about this some more and believe we should be more targeted in the removal:

Suggested change

	rm -r "${wrksrc}/tmp"
	rm -r "${testdir}"

Yes, this will leave tmp around if we created it, but I suspect that won't cause any problems, and it might avoid a problem where a package actually ships a tmp directory. What do you think?

[ahesford]

Alternatively, we could do:

Suggested change

	rm -r "${wrksrc}/tmp"
	rm -r "${testdir}"
	rmdir tmp >/dev/null 2>&1 || true

[tornaria]

I agree with removing only what was created in this run (i.e. just ${tmpdir}).

Also, in terms of avoiding conflicts, maybe the tmpdir should be something more "unique". Perhaps ${wrksrc}/.xbps-tmpdir/$(data +%s) which also has the advantage that pytest will not recurse hidden directories, so the --ignore .. doesn't seem to be necessary.

[icp1994]

I'm a bit confused about the need to remove the $testdir after a successful run. What's the issue if we keep it there?

And if we are adding /.xbps-tmpdir for uniqueness, I don't see why we also need to include another sub-directory under it. I only added the epoch subdirs for the extremely rare chance if a package actually ships a /tmp dirrctory under $wrksrc.

[tornaria]

Fine with me in principle. Although I'd be wary about what happens when there is an old testdir lying around from a previous check run. I don' t know if python -m installer will do the right thing.

[ahesford]

I guess this is irrelevant because the wheel was already built before the tmpdir was created, and the installer should really only be unpacking the wheel. I do prefer @tornaria's suggestion to give the directory a hidden, XBPS-specific name that is extremely unlikely to conflict with package contents. Maybe try changing testdir to .xbps-testdir (which is more meaningful than .xbps-tempdir) and see if that solves your problem without needing to ignore tests, and then skip the removal.

[icp1994]

After some testing, the python installer doesn't override files so moved the deletion logic at top. And removed the now redundant epoch subdir & ignore flag.

[tornaria]

While we are at this, I was going to suggest using a venv instead of --destdir + PYTHONPATH trick. Indeed, I found some packages that fail when PYTHONPATH is messed upon.

The gist of it would be something like:

local testdir="${wrksrc}/.xbps-tmpdir/$(date +%s)"
python3 -m venv --system-site-packages --without-pip ${testdir}
${testdir}/bin/python3 -m installer ${make_install_args} ${make_install_target:-dist/*.whl}

PATH=${testdir}/bin:$PATH ${make_check_pre} python3 -m \
    pytest ${testjobs} ${make_check_args} ${make_check_target}

This is needed to fix check for python3-coverage, see: 4366b1c.

I also needed it to fix check for python3-pytest-cov, however in that case another hack was necessary so I would still need to rewrite do_check(). See: 65eb105.

[ahesford]

I'm not convinced we should move to a venv for testing. Half of your two test cases require further adjustment (symlinking a package into the venv) anyway. Adjusting the Python path is a supported mechanism, so the failure of those two packages probably reflects some inappropriate assumptions in those packages.

[tornaria]

This LGTM, let's get this in, we can figure out whether switching or not to testing using a venv later (cf #44268 where I needed a venv).

[ahesford]

The benefit of cleaning up the test directory after we've used it is that we (attempt to) restore the source tree to the state it was in when the function was first called.

The benefit of cleaning up the directory before it would normally exist seems dubious. If we agree there is no need to clean up the test after a successful run, I think changing the directory name as you've done is sufficient.

[ahesford]

I just ran into the reason for the cleanup: attempting to re-run a check after a failure will cause python3 -m installer to fail unless the test directory is clean. Still, I think the idea of purging a (typically nonexistent) directory just to avoid issues with re-running tests is strange. At this point, I guess I'd rather see

local testdir="${wrksrc}/.xbps-testdir/$(date +s)"

and skip cleanup altogether. This should solve the problem of pytest trying to traverse into tmp without requiring a manual exclude, keeps our tests in a more-likely-unique subdirectory that should not conflict with any package contents, and avoid strange pre-clean steps. As a bonus, if you do run multiple tests to correct a failure, you can always do something like

diff -r .xbps-testdir/<failed> .xbps-testdir/<succeeeded>

after the fact to confirm what, if anything, has changed in the installed wheel.

[icp1994]

Ayyy, feels like I'm running in circles 😆 This should be the Final.Final.Final.patch