Skip to content

treewide: abort -> throw where it makes sense#45637

Closed
infinisil wants to merge 1 commit intoNixOS:masterfrom
infinisil:remove-aborts
Closed

treewide: abort -> throw where it makes sense#45637
infinisil wants to merge 1 commit intoNixOS:masterfrom
infinisil:remove-aborts

Conversation

@infinisil
Copy link
Member

@infinisil infinisil commented Aug 26, 2018 •
edited
Loading

Motivation for this change

I don't really care about this getting merged, but I'd like to find out whether there's any reason for using aborts. Please let me know if you have any valid use case, because I couldn't find any. I also asked many people but nobody had one.

I have NOT tested this in any way, let me know if this breaks anything, which might just be a reason for having aborts then.

Note that I'm looking for actual use cases, not theoretical ones.

Ping @Profpatsch

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)
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: stdenv Standard environment 6.topic: steam Steam game store/launcher (store.steampowered.com) 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Aug 26, 2018
@Ericson2314
Copy link
Member

Ericson2314 commented Aug 26, 2018

Yeah I 've never seen any consistency in one or the other. I assume people use one cause they didn't know of the other.

Mic92
Mic92 reviewed Aug 26, 2018
View reviewed changes
default.nix Outdated
Copy link
Member

@Mic92 Mic92 Aug 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Is this maybe a special case? Some commands like nix-env -qa will silently ignore throw as it issues an assertion error while abort will result in an evaluation error.

Copy link
Member

@Profpatsch Profpatsch Aug 27, 2018

Choose a reason for hiding this comment

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

Yes, this should stay abort.

@Profpatsch
Copy link
Member

Profpatsch commented Aug 26, 2018

As @Mic92 said, abort can’t be skipped, while throw is skipped by nix-env. Most changes from lib signal programmer errors, so should stay abort. It doesn’t make sense for packages to use abort however.

@Mic92
Copy link
Member

Mic92 commented Aug 26, 2018 •
edited
Loading

@Profpatsch may be you point to the examples in lib where abort should be used. Because those are not clear to me.

Profpatsch
Profpatsch suggested changes Aug 27, 2018
View reviewed changes
Copy link
Member

@Profpatsch Profpatsch left a comment

Choose a reason for hiding this comment

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

I commented on the first few changes, it looks to me like nearly all of these are programmer errors that should abort the evaluation whenever they happen (not skippable, like with throw).
So I’m against this change.

default.nix Outdated
Copy link
Member

@Profpatsch Profpatsch Aug 27, 2018

Choose a reason for hiding this comment

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

Yes, this should stay abort.

lib/attrsets.nix Outdated
Copy link
Member

@Profpatsch Profpatsch Aug 27, 2018

Choose a reason for hiding this comment

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

+1

lib/generators.nix Outdated
Copy link
Member

@Profpatsch Profpatsch Aug 27, 2018

Choose a reason for hiding this comment

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

abort, is a programmer error.

lib/generators.nix Outdated
Copy link
Member

@Profpatsch Profpatsch Aug 27, 2018

Choose a reason for hiding this comment

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

abort, is a failed pattern match and thus a programmer error.

lib/generators.nix Outdated
Copy link
Member

@Profpatsch Profpatsch Aug 27, 2018

Choose a reason for hiding this comment

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

as above

lib/modules.nix Outdated
Copy link
Member

@Profpatsch Profpatsch Aug 27, 2018

Choose a reason for hiding this comment

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

Also a programmer error, abort.

lib/options.nix Outdated
Copy link
Member

@Profpatsch Profpatsch Aug 27, 2018

Choose a reason for hiding this comment

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

abort

lib/options.nix Outdated
Copy link
Member

@Profpatsch Profpatsch Aug 27, 2018

Choose a reason for hiding this comment

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

abort

nixos/lib/testing.nix Outdated
Copy link
Member

@Profpatsch Profpatsch Aug 27, 2018

Choose a reason for hiding this comment

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

Programmer error, abort.

@FRidh FRidh added the 9.needs: documentation This needs to be documented well. label Aug 27, 2018
@FRidh
Copy link
Member

FRidh commented Aug 27, 2018

Added "needs: documentation" label because clearly that's the issue here.

@infinisil
Copy link
Member Author

infinisil commented Aug 27, 2018

Ah yes, thanks @Profpatsch! Makes sense for those to be aborts as you explained. I undid those ones.

@infinisil infinisil mentioned this pull request Sep 8, 2018
Closed
@infinisil infinisil changed the title treewide: abort -> throw (aka aborts are useless, convince me otherwise) treewide: abort -> throw where it makes sense Sep 18, 2018
@infinisil
Copy link
Member Author

infinisil commented Sep 18, 2018

Can this be merged like this? This now only changes the aborts that should've been throws from the beginning.

@Mic92
Copy link
Member

Mic92 commented Sep 18, 2018

I am not sure why @edolstra was against the pull request and if those issues are still present.

@infinisil
Copy link
Member Author

infinisil commented Apr 28, 2019

Can't be bothered with this

@infinisil infinisil closed this Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mic92 Mic92 Mic92 left review comments

@edolstra edolstra Awaiting requested review from edolstra

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@nbp nbp Awaiting requested review from nbp

+1 more reviewer

@Profpatsch Profpatsch Profpatsch requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: stdenv Standard environment 6.topic: steam Steam game store/launcher (store.steampowered.com) 8.has: module (update) This PR changes an existing module in `nixos/` 9.needs: documentation This needs to be documented well. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@infinisil @Ericson2314 @Profpatsch @Mic92 @FRidh @GrahamcOfBorg