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

Ensure the sandbox distdir exists when creating dists via PEP517. #16647

Merged
merged 1 commit into from Aug 26, 2022

Conversation

benjyw
Copy link
Sponsor Contributor

@benjyw benjyw commented Aug 25, 2022

Flit, at least, fails if this directory doesn't exist.

Also fixes a docsite text alignment issue that didn't merit its own change.

Also fixes a docsite text alignment issue that didn't merit
its own change.
@benjyw benjyw added the category:bugfix Bug fixes for released features label Aug 25, 2022
@stuhood
Copy link
Sponsor Member

stuhood commented Aug 25, 2022

needs-cherrypick?

@benjyw
Copy link
Sponsor Contributor Author

benjyw commented Aug 25, 2022

PEP 517 does not specify explicitly that the directory must exist, but the example it gives appears to assume that. In any case, there should be no harm in creating it. Setuptools is fine with it and flit requires it.

@@ -154,6 +155,7 @@ class DistBuildResult:
wheel_config_settings = {wheel_config_settings_str}
sdist_config_settings = {sdist_config_settings_str}

os.makedirs(dist_dir, exist_ok=True)
Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fresh sandbox each time, so the exist_ok should not be necessary in practice. But it's good to be defensive, in the unlikely event that a directory called dist does exist in the input snapshot for some reason.

@benjyw benjyw modified the milestones: 2.14.x, 2.13.x Aug 25, 2022
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to cherry-picking. Probably to 2.13?

@benjyw benjyw modified the milestones: 2.13.x, 2.14.x Aug 25, 2022
@benjyw benjyw merged commit 748c281 into pantsbuild:main Aug 26, 2022
@benjyw benjyw deleted the make_sandbox_distdir branch August 26, 2022 04:54
benjyw added a commit to benjyw/pants that referenced this pull request Aug 26, 2022
…ntsbuild#16647)

Also fixes a docsite text alignment issue that didn't merit
its own change.
@benjyw benjyw modified the milestones: 2.14.x, 2.13.x Aug 26, 2022
benjyw added a commit to benjyw/pants that referenced this pull request Aug 26, 2022
…ntsbuild#16647)

Also fixes a docsite text alignment issue that didn't merit
its own change.
benjyw added a commit that referenced this pull request Aug 26, 2022
…rrypick of #16647) (#16659)

Also fixes a docsite text alignment issue that didn't merit its own change.
benjyw added a commit that referenced this pull request Aug 26, 2022
…rrypick of #16647) (#16660)

Also fixes a docsite text alignment issue that didn't merit its own change.
cczona pushed a commit to cczona/pants that referenced this pull request Sep 1, 2022
…ntsbuild#16647)

Also fixes a docsite text alignment issue that didn't merit
its own change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants