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

_build: fix dynamic version when building from sdist #20

Merged
merged 2 commits into from
Oct 13, 2021
Merged

Conversation

FFY00
Copy link
Owner

@FFY00 FFY00 commented Oct 11, 2021

Signed-off-by: Filipe Laíns lains@riseup.net

@FFY00 FFY00 requested a review from henryiii October 11, 2021 02:09
@FFY00 FFY00 changed the title tests: refactor dynamic fixtures _build: fix dynamic version when building from sdist Oct 11, 2021
@FFY00 FFY00 enabled auto-merge (rebase) October 11, 2021 02:21
try:
shutil.rmtree(path)
except PermissionError: # pragma: no cover
pass # this sometimes fails on windows :/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually this means there's a missing close somewhere, say a file opened without a context manager, etc. Only Windows cares about files being closed before deletion, so it only shows up on Windows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In trampolim/_sdist.py, there are two manual calls to .close() without context managers. Context managers should be used, or they should be in a try:/finally: block. The files might not be closed if an error is thrown.

Copy link
Owner Author

Choose a reason for hiding this comment

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

That makes the code slightly harder to read, as it introduces two indentation levels. However, we are not handling the closing in case of an exception, so we can use https://docs.python.org/3/library/contextlib.html#contextlib.closing to fix that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use contextlib.ExitStack to avoid nested exceptions, like I did in #21 (though I didn't realize that the source context manager didn't cover the whole function, so it does need one extra indentation level).

You don't need to wrap tar file.TarFile in a closing context manager, it already has one since Python 3.2. GzipFile does seem to be missing a context manager, so it does a closing wrapper.

Copy link
Collaborator

@henryiii henryiii Oct 11, 2021

Choose a reason for hiding this comment

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

This try/except needs to be reverted to the version on main. It's hiding potential valid errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is 3.10 passing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Session test-3.10 skipped: Python interpreter 3.10 not found.

Oh. That's why.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yup. If you want to fix that...


I don't know where the issue is, it errors out as

PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\trampolim-test-s_oya_bn\\no-version-1.0.0+custom'

Which is a directory. I don't have time to track it down right now, so this will have to be on hold for a little while.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll try to turn on my Windows box and see if I can debug locally soonish.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thank you 🙏

trampolim/_sdist.py Outdated Show resolved Hide resolved
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

LGTM!

@FFY00 FFY00 merged commit cc179bc into main Oct 13, 2021
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.

2 participants