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

Jump to bottom

stdenv, cacert: $NIX_SSL_CERT_FILE changes #61179

Merged
merged 3 commits into from May 19, 2019
Merged

stdenv, cacert: $NIX_SSL_CERT_FILE changes #61179

merged 3 commits into from May 19, 2019

Conversation

vcunat
Copy link
Member

@vcunat vcunat commented May 9, 2019

Motivation for this change

Some SSL libs don't react to $SSL_CERT_FILE. That actually makes sense to me, as we add this behavior as nixpkgs-specific, so it seems "safer" to use $NIX_*.

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 nix-review --run "nix-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: docs are still missing ;-)
  • Fits CONTRIBUTING.md.

@vcunat
stdenv, cacert: consider $NIX_SSL_CERT_FILE in hooks
79bd4ad 
Some SSL libs don't react to $SSL_CERT_FILE.
That actually makes sense to me, as we add this behavior
as nixpkgs-specific, so it seems "safer" to use $NIX_*.
This was referenced May 9, 2019
Open
Merged
layus
layus approved these changes May 9, 2019
View reviewed changes
Copy link
Member

@layus layus left a comment

Choose a reason for hiding this comment

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

It totally makes sense, and should have been done before :-)

pkgs/stdenv/generic/setup.sh Show resolved Hide resolved
vcunat added 2 commits May 9, 2019 09:49
@vcunat
stdenv: also override cert files in pure nix-shell
b27cc37 
That's very much consistent with the spirit of nix-shell --pure

BTW, nix 1.x shells will be always treated as pure;
in that version detection isn't possible.
NixOS/nix@1bffd83e1a9c
@vcunat
pypi2nix: fix $IN_NIX_SHELL test
99760ed 
In nix 2.0 this changed: NixOS/nix@1bffd83
I only kept the original intention and did no kind of verification.
@vcunat
Copy link
Member Author

vcunat commented May 9, 2019

/cc pypi2nix maintainer @garbas. It couldn't work for years in the intended way, so I don't know.

@ofborg ofborg bot requested a review from garbas May 9, 2019 08:16
@vcunat vcunat changed the title stdenv, cacert: consider $NIX_SSL_CERT_FILE in hooks stdenv, cacert: $NIX_SSL_CERT_FILE changes May 9, 2019
@LnL7
Copy link
Member

LnL7 commented May 9, 2019

My reasoning back when I added it was to go trough the upstream codepath instead of our patches by default. But that doesn't really make sense since we want NIX_SSL_CERT_FILE to work for everything.

@vcunat vcunat merged commit 99760ed into NixOS:staging May 19, 2019
vcunat added a commit that referenced this pull request May 19, 2019
@vcunat
Merge #61179: stdenv, cacert: $NIX_SSL_CERT_FILE changes
96a6043 
... into staging
@vcunat vcunat deleted the p/cacert-NIX_SSL branch May 19, 2019 12:48
vcunat added a commit that referenced this pull request May 19, 2019
@vcunat
gnutls: fix tests after 79bd4ad (PR #61179)
347cd8a 
It's one of the places that would reach out to /etc/ otherwise,
so I expect we have to pay this price to get the effect.
Hopefully there won't be too many places to patch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@layus layus layus approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314 Ericson2314 is a code owner

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@garbas garbas Awaiting requested review from garbas

Assignees
No one assigned
Labels
6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vcunat @LnL7 @layus