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

rsync: Enable the --ignore-case patch #52630

Closed
wants to merge 1 commit into from
Closed

Conversation

Baughn
Copy link
Contributor

@Baughn Baughn commented Dec 21, 2018

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Baughn
Copy link
Contributor Author

Baughn commented Dec 21, 2018

Somehow, this patch reduces the closure size. (By 0.02%)

@Baughn
Copy link
Contributor Author

Baughn commented Dec 21, 2018

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";
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

@Mic92
Copy link
Member

Mic92 commented Dec 21, 2018

That should also go to staging.

@Baughn
Copy link
Contributor Author

Baughn commented Dec 22, 2018

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.)

@Baughn
Copy link
Contributor Author

Baughn commented Dec 22, 2018

@Baughn
Copy link
Contributor Author

Baughn commented Dec 22, 2018

So, to clarify, I should reopen this PR against staging-next?

@xeji
Copy link
Contributor

xeji commented Jan 5, 2019

So, to clarify, I should reopen this PR against staging-next?

No. Please rebase this PR on the current staging branch. No need to open a new PR.

@xeji
Copy link
Contributor

xeji commented Jan 5, 2019

A quick explanation: all PRs that trigger a high number of rebuilds should go to the staging branch so we can bundle multiple mass-rebuilding changes. Rebuilding a large number of packages on master for a single PR would be inefficient and delay development of other changes.

From staging, a bundle of changes will be merged to staging-next and eventually to master when it has stabilized on our CI. The details are explained in the workflow document linked above.

@mmahut
Copy link
Member

mmahut commented Aug 13, 2019

Are there any updates on this pull request, please?

@Baughn
Copy link
Contributor Author

Baughn commented Aug 14, 2019

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.

@Baughn Baughn closed this Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants