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

whipper: 0.7.0 -> 0.7.2 #51302

Merged
merged 1 commit into from Dec 2, 2018

Conversation

Projects
None yet
6 participants
@Moredread
Copy link
Contributor

Moredread commented Dec 1, 2018

Motivation for this change
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.

@@ -3,13 +3,13 @@

python2.pkgs.buildPythonApplication rec {
name = "whipper-${version}";
version = "0.7.0";
version = "0.7.2";

This comment has been minimized.

@Mic92

Mic92 Dec 1, 2018

Contributor

The following should make the patch unnecessary:

makeWrapperArgs = [
  "--prefix" "PATH" ":" "${stdenv.lib.makeBinPath [ utillinux ]}"
];
@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Dec 1, 2018

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/ofborg-now-uses-checks/1558/1

@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Dec 1, 2018

@Moredread Moredread force-pushed the Moredread:whipper/0.7.2 branch Dec 1, 2018

@Moredread Moredread force-pushed the Moredread:whipper/0.7.2 branch to cb452d0 Dec 1, 2018

@Moredread

This comment has been minimized.

Copy link
Contributor Author

Moredread commented Dec 1, 2018

@jtojnar @Mic92: Done. Any idea how we can eliminate the patch for cdparanoia? Whipper expects it as cd-paranoia.

@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Dec 1, 2018

I actually prefer the patch approach to the wrapper – it ensures the programs are still necessary. That is actually why I introduced it. Though if the whipper maintainers prefer the wrapper method, they can feel free to change it.

@Moredread

This comment has been minimized.

Copy link
Contributor Author

Moredread commented Dec 1, 2018

@jgeerds @rycee what is your opinion on this?

Edit: I personally prefer avoiding patches as they make updating packages much harder when they don't apply correctly anymore.

@rycee

This comment has been minimized.

Copy link
Member

rycee commented Dec 2, 2018

I kind of prefer the wrapper approach where possible because of the lessened maintenance burden. I do see the benefit of immediately being aware when a dependency becomes unnecessary but I think the ease of maintenance overweights this.

@rycee

This comment has been minimized.

Copy link
Member

rycee commented Dec 2, 2018

Merged to master.

@rycee rycee closed this Dec 2, 2018

@rycee rycee merged commit cb452d0 into NixOS:master Dec 2, 2018

9 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Mic92

This comment has been minimized.

Copy link
Contributor

Mic92 commented Dec 2, 2018

Yes. It is too easy to miss a spot where a program was also called in my opinion. Especially in the presence of automated updates.

@Moredread Moredread deleted the Moredread:whipper/0.7.2 branch Dec 3, 2018

@nixos-discourse

This comment was marked as spam.

Copy link

nixos-discourse commented Dec 25, 2018

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/runtime-dependency-on-an-executable/1759/2

2 similar comments
@nixos-discourse

This comment was marked as spam.

Copy link

nixos-discourse commented Dec 25, 2018

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/runtime-dependency-on-an-executable/1759/2

@nixos-discourse

This comment was marked as spam.

Copy link

nixos-discourse commented Dec 25, 2018

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/runtime-dependency-on-an-executable/1759/2

@nixos-discourse

This comment was marked as spam.

Copy link

nixos-discourse commented Dec 25, 2018

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/discourse-github-integration/1438/6

1 similar comment
@nixos-discourse

This comment was marked as spam.

Copy link

nixos-discourse commented Dec 25, 2018

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/discourse-github-integration/1438/6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.