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

slack: Return a dummy derivation on unsupported platforms #88727

Closed
wants to merge 1 commit into from

Conversation

@adisbladis
Copy link
Member

adisbladis commented May 23, 2020

Without --show-trace the Slack derivation throws an error where it's
not clear which package throws the error.

Returning a dummy derivation with platforms set to [] results in a
friendlier error message with a lot more context.

Motivation for this change
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.
Without `--show-trace` the Slack derivation throws an error where it's
not clear which package throws the error.

Returning a dummy derivation with platforms set to `[]` results in a
friendlier error message with a lot more context.
in {
x86_64-darwin = darwin;
x86_64-linux = linux;
}.${system} or dummyDerivation

This comment has been minimized.

Copy link
@flokli

flokli May 23, 2020

Contributor
Suggested change
}.${system} or dummyDerivation
}.${system} or throw "No slack derivation for ${stdenv.hostPlatform.system}"

This comment has been minimized.

Copy link
@adisbladis

adisbladis May 23, 2020

Author Member

This is still not friendly enough IMO and inconsistent with how other derivations behave when trying to build for an unsupported platform.

Simply throwing an error will cause the package not to show up in nix-env -qaP which is confusing.

This comment has been minimized.

Copy link
@flokli

flokli May 24, 2020

Contributor

I wouldn't say its inconsistent with how other derivations handle building for unsupported platforms. In addition to setting meta.plaforms, throwing a "unsupported platform" message seems quite common:

  • pkgs/applications/graphics/unigine-valley/default.nix
  • pkgs/applications/misc/playonlinux/default.nix
  • pkgs/development/compilers/cmucl/binary.nix
  • pkgs/development/compilers/openjdk/openjfx/11.nix
  • pkgs/development/compilers/openjdk/openjfx/13.nix
  • pkgs/development/compilers/openjdk/8.nix
  • pkgs/development/compilers/sagittarius-scheme/default.nix
  • pkgs/development/libraries/xalanc/default.nix
  • pkgs/development/pharo/vm/build-vm.nix
  • pkgs/development/tools/poetry2nix/poetry2nix/pep508.nix
  • pkgs/games/steam/default.nix
  • pkgs/games/vessel/default.nix
  • pkgs/shells/powershell/default.nix
  • pkgs/tools/security/enpass/default.nix
  • pkgs/top-level/lua-packages.nix

There's even a x = {…}.${system} or throwSystem; pattern used in some places:

  • pkgs/applications/blockchains/mist.nix
  • pkgs/applications/misc/simplenote/default.nix
  • pkgs/applications/networking/instant-messengers/slack/default.nix
  • pkgs/applications/networking/instant-messengers/wire-desktop/default.nix
  • pkgs/development/libraries/oracle-instantclient/default.nix
  • pkgs/development/tools/electron/3.x.nix

(including further up in the slack package code)

Building a dummy derivation, only for the purpose of it failing a build looks more hacky to me.

@flokli
Copy link
Contributor

flokli commented Jun 6, 2020

@adisbladis any further comments? I really think the way it currently is done is consistent with the rest of nixpkgs, and if we want a better error message, this should be a nixpkgs-wide (or nix-wide) improvement.

@adisbladis
Copy link
Member Author

adisbladis commented Jun 6, 2020

I still don't like throw as opposed to setting meta.platforms, the latter is far easier for a user to fix by themselves while being able to reuse a derivation.

I do agree it's a larger issue though and should be fixed in a more consistent manner.

@adisbladis adisbladis closed this Jun 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.