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

Bump powershell #86423

Merged
merged 2 commits into from May 4, 2020
Merged

Bump powershell #86423

merged 2 commits into from May 4, 2020

Conversation

jonringer
Copy link
Contributor

@jonringer jonringer commented Apr 30, 2020

Motivation for this change

closes: #84454

enables darwin usage

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.

Package and openssl dependency version bump and cascading changes.
@SRGOM
Copy link
Member

SRGOM commented May 2, 2020

Hi, thanks for this. Sorry I couldn't look earlier (Might sound luddite but I don't check email regularly ... :( )

@SRGOM
Copy link
Member

SRGOM commented May 3, 2020

I reviewed: e4bb796

Instructions for a first time reviewer (such as me):

git remote add someRandomName  <repo=https://github.com/jonringer/nixpkgs>
git fetch --depth 2 sameRandomName <commitid=e4bb796e3e16f8186d9ff8ca16bb18134a9345fb>
git checkout <commitid=e4bb796e3e16f8186d9ff8ca16bb18134a9345fb >

Checking the diffs in vim with whitespace removed shows that the only changes are:

  1. removing the -f flag from rm
  2. making the file extension platform dependant.

I can confirm that this is fine for Linux.

@jonringer
Copy link
Contributor Author

Yea, i was able to get it to work fine on Linux as well. I just want to ensure that I'm not causing a regression to darwin, which previously worked.

Wish they would use a newer version of openssl by default. Would make a lot of this a lot easier

@ofborg ofborg bot requested a review from SRGOM May 3, 2020 18:43
@jonringer jonringer mentioned this pull request May 3, 2020
10 tasks
@SRGOM
Copy link
Member

SRGOM commented May 4, 2020

Using a passed library and gating the deletion on non-darwin env.

Presuming, (somewhat obviously) that you've tested this on Darwin. Approved.

Copy link
Member

@SRGOM SRGOM left a comment

Choose a reason for hiding this comment

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

I see in vi doInstallCheck and an installCheck added. Approved.

@jonringer
Copy link
Contributor Author

passes on darwin, merging :)

@jonringer jonringer merged commit 5959a5b into NixOS:master May 4, 2020
@jonringer jonringer deleted the bump-powershell branch May 4, 2020 16:25
@dasJ
Copy link
Member

dasJ commented May 23, 2020

@jonringer Does this qualify for a backport to 20.03? It doesn't eval there because OpenSSL is insecure.

@SRGOM
Copy link
Member

SRGOM commented May 26, 2020

Don't know the backporting policy but @dasJ you can consider using unstable for powershell.

@jonringer
Copy link
Contributor Author

@dasJ Unless there's a major security vulnerability, I think not issuing another major version would be wise for the release branch

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

3 participants