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
rsync: Enable the --ignore-case patch #52630
Conversation
Somehow, this patch reduces the closure size. (By 0.02%) |
nox-review doesn't work. It attempts to run ~every test, which immediately OOMs. |
patches = stdenv.lib.optional enableCopyDevicesPatch "./patches/copy-devices.diff"; | ||
srcs = [mainSrc patchesSrc]; | ||
patches = stdenv.lib.optional enableCopyDevicesPatch "./patches/copy-devices.diff" | ||
++ stdenv.lib.optional enableIgnoreCasePatch "./patches/ignore-case.diff"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forgot to add the patch itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, it is part of rsync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a so-called "maintained patch". So it should keep working, it just won't be applied.
Which, yes, is a little worrying. I don't know why they're doing it that way, and there are no hints in the diff itself. To quote:
This adds the --ignore-case option, which makes rsync compare filenames
in a case-insensitive manner.
To use this patch, run these commands for a successful build:
patch -p1 <patches/ignore-case.diff
./configure (optional if already run)
make
TODO:
- Make this code handle multibyte character encodings, and honor the
--iconv setting when converting case.
based-on: d73762eea3f15f2c56bb3fa9394ad1883c25c949
That's all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...but I skimmed the code, and I didn't see anything questionable. Here: http://ix.io/1wwQ
That should also go to staging. |
Sorry, I'm not sure what you mean. (What is staging, and how does it get involved? Did the workflow change? I may have missed something.) |
I've been pointed at https://github.com/NixOS/rfcs/blob/master/rfcs/0026-staging-workflow.md. |
So, to clarify, I should reopen this PR against staging-next? |
No. Please rebase this PR on the current |
A quick explanation: all PRs that trigger a high number of rebuilds should go to the From |
Are there any updates on this pull request, please? |
The patch doesn't quite do what I wanted it to do. What it does do isn't nearly as useful. Hence, I don't think there's reason to include it. Closing accordingly. |
Motivation for this change
--ignore-case is a useful rsync option, performing a function that people keep asking for. There should be no downside to applying it by default, as it's still gated behind a flag.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)