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.

Sign up for GitHub

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

Jump to bottom

Added reflinking as option for the post processor #3394

Closed
wants to merge 2 commits into from
Closed

Added reflinking as option for the post processor #3394

wants to merge 2 commits into from

Conversation

Kriskras99
Copy link
Contributor

  • PR is based on the DEVELOP branch
  • Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
  • Read the contribution guide

This adds reflink support for CoW filesystems, currently only BTRFS, upstream is working on adding support for Apple's APFS and I'm looking into Windows' ReFS.

medariox
medariox requested changes Dec 3, 2017
View reviewed changes
Copy link
Contributor

@medariox medariox left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the contribution. 👍

medusa/helpers/__init__.py Outdated
:type dest_file: str
"""
try:
reflink.reflink(str(src_file), str(dest_file))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the str() here?

requirements.txt Outdated
@@ -20,6 +20,7 @@ profilehooks==1.9.0
PyGithub==1.35
PyNMA==1.0
python-dateutil==2.6.1
reflink=0.1.4
Copy link
Contributor

Choose a reason for hiding this comment

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

Double == instead of single =. You also need to include the reflink package in the ext folder, as we currently still bundle external packages.

@fernandog
Copy link
Contributor

I would add a note quick explaining this setting

@Kriskras99
Copy link
Contributor Author

@fernandog Were would you suggest adding this note?
@medariox What would be the best way to bundle cffi?

@medariox
Copy link
Contributor

medariox commented Dec 4, 2017

@Kriskras99
I'm afraid we can't add packages that have C dependencies until we have a proper setup script. 😞

@fernandog
Copy link
Contributor

@Kriskras99 I would add here: https://github.com/pymedusa/Medusa/pull/3394/files#diff-78821c2496564ec7b9c01846a747a2caL79

@medariox what do you think?

@medariox
Copy link
Contributor

medariox commented Dec 4, 2017

@fernandog
Sure, but the main issue is that we currently can't add the cffi dependency.

@labrys
Copy link
Contributor

labrys commented Dec 4, 2017

@medariox it could be added as an optional feature that you have to manually install the package and it uses it only if available, similar to mediainfo.

@Kriskras99 Kriskras99 closed this Jan 19, 2018
@Kriskras99 Kriskras99 mentioned this pull request Jan 19, 2018
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@medariox medariox medariox requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Kriskras99 @fernandog @medariox @labrys