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

libxml2,libxslt: fix pythonSupport=false override #83737

Merged
merged 1 commit into from Apr 5, 2020
Merged

libxml2,libxslt: fix pythonSupport=false override #83737

merged 1 commit into from Apr 5, 2020

Conversation

veprbl
Copy link
Member

@veprbl veprbl commented Mar 30, 2020

Motivation for this change

Fixes: #73102
Also see: #53487 and #36229

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Tested with

with import ./. {
  config = {
    packageOverrides = pkgs: {
      libxslt = pkgs.libxslt.override { pythonSupport = false; };
      libxml2 = pkgs.libxml2.override { pythonSupport = false; };
    };
  };
};

libxslt

@veprbl veprbl requested a review from FRidh March 30, 2020 02:19
gnprice
gnprice reviewed Apr 2, 2020
View reviewed changes
Copy link
Contributor

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

This looks like a good change!

It'd be great if you can add a sentence or two about what this is fixing, and a link to #73102 and/or one of the other linked issues. I found those issues highly educational to read; and the commit message can make sure that someone like me coming along a bit later and finding this change in git log, instead of finding it now as a PR, can benefit the same way.

pkgs/development/libraries/libxslt/default.nix Show resolved Hide resolved
@veprbl
Copy link
Member Author

veprbl commented Apr 2, 2020

@gnprice Regarding the links to the issues: GitHub saves correspondence between commits on repo branches and merged pull requests. This information is stored in commit messages unless pull request is merged via "Rebase and merge", then GitHub only tracks the correspondence internally, but you can still access it via the GitHub web interface. Adding links manually creates unnecessary notification noise in the issues, so I prefer to avoid that.

@gnprice
Copy link
Contributor

gnprice commented Apr 2, 2020

Adding links manually creates unnecessary notification noise in the issues, so I prefer to avoid that.

OK. In my own projects, I regret that noise and wish GitHub did a better job with it, but I so much value the experience of reading the Git history that I tolerate it as the price to pay for helping there. (It's true that the GitHub web interface will map a commit to the PR that merged it, and that in turn can link to the issue... but those extra indirections mean three slow GitHub page loads instead of one.) I understand making a different choice, though.

It'd still be great if you can add a sentence or two of explanation -- doubly so when there isn't a link straight to the issue 🙂 . That way the reader can see at least a summary of what the idea is, and only has to track down the issue if they want to go deeper into the details.

@veprbl
Copy link
Member Author

veprbl commented Apr 2, 2020

@gnprice Sure. There is always a way...

@veprbl veprbl merged commit e163a36 into NixOS:master Apr 5, 2020
@veprbl veprbl deleted the pr/libxslt_override_fix branch April 5, 2020 03:31
@veprbl veprbl restored the pr/libxslt_override_fix branch December 1, 2020 16:47
@veprbl veprbl deleted the pr/libxslt_override_fix branch December 1, 2020 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnprice gnprice gnprice left review comments

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Impossible to override libxml2 and libxslt with pythonSupport = false
3 participants
@veprbl @gnprice @FRidh