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

Don't force-break SSL applications in shells #15571

Merged
merged 1 commit into from Sep 1, 2016

Conversation

abbradar
Copy link
Member

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

Fixes #13744.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @edolstra, @vcunat and @urkud to be potential reviewers

@peti
Copy link
Member

peti commented May 20, 2016

Why does the generic builder set that value of $SSL_CERT_FILE at all? What is wrong with letting curl and friends use the built-in default cert file?

@abbradar
Copy link
Member Author

cc @edolstra as the author of original 917ca89. I suppose there were purity issues with it because applications could get SSL bundle from the host OS X or Linux system.

@edolstra
Copy link
Member

This change makes Nix shells less pure. Whether is desirable to have predictable SSL behaviour in nix-shell is subjective I guess.

@vcunat
Copy link
Member

vcunat commented May 20, 2016

It would be best to depend on passing --pure.

@abbradar
Copy link
Member Author

Yeah, it would be the best choice. I did patch this way because there's no way I know to distinguish one from another.

@vcunat
Copy link
Member

vcunat commented May 22, 2016

Well, setting $SSL_CERT_FILE explicitly in your environment would achieve that (e.g. in ~/.profile), as without --pure the variable would get passed and stdenv code wouldn't touch it... but it doesn't feel like good solution either.

@vcunat
Copy link
Member

vcunat commented May 23, 2016

Nitpick: the syntax seems bad.

setup: line 392: [: missing `]'

@abbradar
Copy link
Member Author

abbradar commented May 23, 2016

Ouch! I remember running some build and stopping as soon as it got to configure (Perl, I think) but I haven't considered that it may, e.g., use its own builder. Wlll fix...

@vcunat
Copy link
Member

vcunat commented May 23, 2016

It doesn't break builds. I don't know why. (I'm running on a system that includes this PR.)

@abbradar
Copy link
Member Author

Hm, how have you got this message?

@vcunat
Copy link
Member

vcunat commented May 23, 2016

The message is in every build log, but it doesn't make the builds fail. Perhaps just the if fails or something...

@abbradar
Copy link
Member Author

Indeed,

$ [ "" = "" && "" = "" ]
bash: [: missing `]'
$ [ "" = "" ] && [ "" = "" ]
$

@abbradar
Copy link
Member Author

Updated and this time the snippet was tested in a Bash terminal.

@vcunat
Copy link
Member

vcunat commented May 23, 2016

Fun hack: if [ -z "$SSL_CERT_FILE$IN_NIX_SHELL" ]; then

@vcunat
Copy link
Member

vcunat commented Jun 9, 2016

See NixOS/nix#933

@danbst
Copy link
Contributor

danbst commented Aug 31, 2016

@domenkozar @edolstra can we get this into 16.09? I just hit this on freshly deployed unstable, but didn't hit this on 16.03.

@domenkozar
Copy link
Member

+1 for better UX on this front.

@domenkozar domenkozar added this to the 16.09 milestone Aug 31, 2016
@vcunat
Copy link
Member

vcunat commented Aug 31, 2016

This is waiting for NixOS/nix#933 and/or NixOS/nix#1027 – the latter doesn't seem catchable before 16.09, but how can I know...

@edolstra
Copy link
Member

Well, we could do another 1.11 maintenance release with the IN_NIX_SHELL change, but since it's a potential breaking change, I'm not sure if it's a good idea...

Another possibility is to have nix-shell propagate SSL_CERT_FILE, just like it does for TZ.

@vcunat
Copy link
Member

vcunat commented Aug 31, 2016

@edolstra: we can bump to 1.12 if you prefer. As for nixpkgs 16.09, I think we can squeeze this kind of breaking change.

@vcunat
Copy link
Member

vcunat commented Aug 31, 2016

BTW, getting rid of perl seems quite a major change (much larger), even if it doesn't break compatibility.

@edolstra
Copy link
Member

edolstra commented Sep 1, 2016

Sure, for 1.12 it's fine, but that's not going to be released any time soonish.

@domenkozar
Copy link
Member

Merging to staging, hopefully we can get this one to 16.09 with minimal impact.

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

7 participants