Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rely on session context when building sdist and wheel #3060

Merged
merged 1 commit into from Feb 2, 2022

Conversation

jaraco
Copy link
Member

@jaraco jaraco commented Jan 29, 2022

Summary of changes

Closes #3059

Pull Request Checklist

@jaraco jaraco closed this Jan 29, 2022
@jaraco jaraco reopened this Jan 29, 2022
@jaraco
Copy link
Member Author

jaraco commented Jan 29, 2022

Github has messed up this PR. The diff is wrong and there are too many commits. If I look at the branch, it shows correctly that this branch is one commit ahead of main:

image

That's not pertinent to the failures, but it surely doesn't help in the analysis.

@jaraco jaraco force-pushed the bugfix/3059-simple-session-build branch from 48f463f to 26652b4 Compare January 29, 2022 23:46
@jaraco
Copy link
Member Author

jaraco commented Jan 30, 2022

Aha. I did some searching and found this article, which looks suspiciously similar, so I suspect its where the inspiration was drawn from, and confirms my misunderstanding about session fixtures. They're not automatically synchronized across workers in xdist, so each worker attempts to generate their own.

@jaraco
Copy link
Member Author

jaraco commented Jan 30, 2022

I distilled the two issues I found with that article.

@jaraco jaraco force-pushed the bugfix/3059-simple-session-build branch from 26652b4 to 588c322 Compare January 30, 2022 02:10
@jaraco jaraco force-pushed the bugfix/3059-simple-session-build branch from 588c322 to 253ecb0 Compare January 30, 2022 02:18
@abravalheri
Copy link
Contributor

abravalheri commented Feb 1, 2022

Hi @jaraco, thank you very much for the deep investigation.
Indeed, I did not realise the shortcoming of this technique...

I understand that this PR will solve the problem if xdist is not used, but when xdist is being used, things remain more or less the same. Is that correct?

Does it make sense / is it worthy to manually remove the created directory after the yield inside the context?

locked_dir = shared_dir / name
with FileLock(locked_dir.with_suffix(".lock")):
# ^-- prevent multiple workers to access the directory at once
locked_dir.mkdir(exist_ok=True, parents=True)
yield locked_dir

and transform setuptools_wheel and setuptools_sdist in "yield fixtures"? Would that solve the problem when using xdist?

@jaraco
Copy link
Member Author

jaraco commented Feb 2, 2022

I understand that this PR will solve the problem if xdist is not used, but when xdist is being used, things remain more or less the same. Is that correct?

Yes. That's right. As I was investigating, I learned that tmp_path_factory.getbasetemp() changes depending on whether xdist is enabled, and it was only in the case that xdist was disabled or using only one process that the state was getting stored outside the session dir.

Does it make sense / is it worthy to manually remove the created directory after the yield inside the context?

Originally, I was concerned, but once I figured out the basedir mismatch and also learned that pytest normally leaves files behind to clean them up in the future, so I'm okay with it now that it gets cleaned up with or without xdist.

@jaraco jaraco merged commit 7956c10 into main Feb 2, 2022
@jaraco jaraco deleted the bugfix/3059-simple-session-build branch February 2, 2022 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some setuptools tests run against stale artifacts
2 participants