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

Exclude .venv from Time Machine backups #4599

Merged
merged 16 commits into from
Jun 5, 2022
Merged

Exclude .venv from Time Machine backups #4599

merged 16 commits into from
Jun 5, 2022

Conversation

probablykasper
Copy link
Contributor

@probablykasper probablykasper commented Oct 6, 2021

  • Added tests for changed code.
  • Updated documentation for changed code.

By default .venv folders are in ~/Library/Caches, but they can also be stored in project folders using the virtualenvs.in-project config option. When this happens, cache is being stored in a non-standard location, so this PR makes sure backup solutions like Time Machine know not to back it up.

Adds an com.apple.metadata:com_apple_backup_excludeItem xattr for .venv folders on macOS. This means .venv folders won't be included in Time Machine backups (and other backup solutions respecting this attribute).

This is the same as what Rust's package manager does for their target folder.

Copy link

@Jc2k Jc2k left a comment

Choose a reason for hiding this comment

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

I'm just another user, but would like this feature too so thought i'd give it a quick review.

Do you have any references for where com.apple.metadata:com_apple_backup_excludeItem comes from and how stable it is likely to be? Looking at cargo here it uses CFURLSetResourcePropertyForKey rather than xattr directly. I agree xattr is the way to go, especially as the objc libs for python are huge, but a few links to other projects relying on setting or checking the xattr directly might help other reviewers to verify that the change is correct.

tests/utils/test_env.py Outdated Show resolved Hide resolved
poetry/utils/env.py Outdated Show resolved Hide resolved
poetry/utils/env.py Outdated Show resolved Hide resolved
@probablykasper
Copy link
Contributor Author

@Jc2k When you run tmutil addexclusion <path>, it adds this xattr (man tmutil calls it a "sticky" exclusion). I haven't managed to find any official documentation for com.apple.metadata:com_apple_backup_excludeItem though, just people stating it's a thing.

The value I'm setting in the code is copied from what tmutil addexclusion sets, and it also matches what Rust sets. Tried running tmutil isexcluded ./.venv and it says it's excluded.

@apollo13
Copy link
Contributor

apollo13 commented Oct 7, 2021

xattr seems to only ship wheels for macos, which means it would require compilation on windows/linux. At the very least I'd think this package should only get installed on macos since it is only used there…

@probablykasper
Copy link
Contributor Author

@apollo13 Sure, I think now it won't install anywhere except on darwin

@finswimmer
Copy link
Member

Hello everyone,

thank you all for your interest in contributing to poetry. We really appreciate it 👍

However, I don't think we will implement these changes. It should be the job of the third-party tool, to decide what it will do with a file it consumes and not the job of the application that creates the file. Especially in this case here the third-party tool is completely unrelated.

Thanks again for the time you've already invested!

fin swimmer

@probablykasper
Copy link
Contributor Author

probablykasper commented Oct 7, 2021

@finswimmer I disagree with that. By default Poetry stores .venv in ~/Library/Caches, which is indeed ignored by any sensible backup tool. But Poetry also lets you store .venv in project folders (Forgot this isn't default, should've explained when I opened the PR).

When tools put cache in a non-standard location, they should make sure it's treated correctly, like Rust does. I don't think it's reasonable to put the burden on backup solutions to figure out how every package manager out there works.

@probablykasper
Copy link
Contributor Author

In addition to cargo (Rust), it looks like npm (Node.js) plans to implement this for node_modules in npm 9.

@finswimmer Would love to know what you think, and what others think if you're still skeptical about it. Thanks!

@neersighted
Copy link
Member

In addition to cargo (Rust), it looks like npm (Node.js) plans to implement this for node_modules in npm 9.

@finswimmer Would love to know what you think, and what others think if you're still skeptical about it. Thanks!

I'm generally with @finswimmer on this one -- it seems inelegant and a bit out of scope. I'd love to see links to implementations in other language's package managers to get an idea of what the wider world thinks of this.

@probablykasper
Copy link
Contributor Author

@neersighted Could you please elaborate on how it's inelegant and out of scope?

When you store cache outside of a cache folder, I see it as a responsibility to make sure it's properly treated as cache. In this case, there's luckily a conventional way to go about doing this.

The only alternative I see is to let all the backup services out there learn how every single package manager/tool/etc stores cache, and risk potentially ignoring folders they shouldn't. That, I definitely don't think is elegant.

@neersighted
Copy link
Member

My rationale is that this is historically out of scope, and that backup tools generally do not hoover up dotfiles. Obviously times change and my sensibilities are probably rooted a bit in Unix neckbeard-ism.

This seems more involved/elaborate than I would like, but your mention of other projects is a point in favor of the change. I'm not necessarily opposed -- but I'd like to see the consensus from similar projects to help inform my judgement.

@probablykasper
Copy link
Contributor Author

backup tools generally do not hoover up dotfiles

Didn't think about that, but it depends on the purpose of the backup tool. If you have a full or bootable backup, you definitely want to include dotfiles since lots of tools use dotfiles for configs (git, npm, docker etc).

This seems more involved/elaborate than I would like

I don't really think so, unless I'm misunderstanding you. It's a simple folder xattr, with a basic plist-encoded string value.

You can see Rust's implementation here (that function is called here). They're directly calling the macOS Obj-C API which adds this.
Original issue requesting this: rust-lang/cargo#3884
2019 issue requesting it to be fixed due to regression: rust-lang/cargo#7189

poetry/utils/env.py Outdated Show resolved Hide resolved
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Can we add a TODO comment to add some backup-ignore marker for other platforms, and then rebase this?

@neersighted neersighted added this to the 1.2 milestone Jun 4, 2022
@probablykasper
Copy link
Contributor Author

Sure thing, good now @neersighted?

# Exclude the venv folder from from macOS Time Machine backups
# TODO: Add backup-ignore markers for other platforms too
if sys.platform == "darwin":
import xattr # type: ignore[import]
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding a # type: ignore comment let's add this to the Mypy settings in pyproject.toml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, added it there instead :)

pyproject.toml Outdated Show resolved Hide resolved
@neersighted neersighted merged commit ca76e11 into python-poetry:master Jun 5, 2022
@abn abn mentioned this pull request Jun 6, 2022
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants