Skip to content

nixos-version: use version_ instead of version #50364

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

nixos-version: use version_ instead of version #50364

wants to merge 1 commit into from
+3 −2

Conversation

Synthetica9
Copy link
Member

@Synthetica9 Synthetica9 commented Nov 14, 2018 •
edited
Loading

Related: 1e99582

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

Sorry, something went wrong.

@Synthetica9
nixos-version: use version_ instead of version

Verified

This commit was signed with the committer’s verified signature. The key has expired.
Synthetica9 Patrick Hilhorst
GPG key ID: 589BB0A8DAFEF2B2
Expired
Verified
Learn about vigilant mode
7f2756c 
Related: 1e99582
@GrahamcOfBorg GrahamcOfBorg added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Nov 14, 2018
@matthewbauer
Copy link
Member

I don't think we should have to do this. Doesn't version still get in the derivation attrs?

@Synthetica9
Copy link
Member Author

Yes, but as it were before, it cascaded through to mkDerivation, which threw an error. See also: #47883 (comment)

@GrahamcOfBorg GrahamcOfBorg added 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 Nov 14, 2018
@matthewbauer
Copy link
Member

Can’t we just fix mkDerivation to not throw that error?

This was referenced Nov 25, 2018
Merged
Closed
@cyounkins cyounkins mentioned this pull request Nov 26, 2018
Merged
10 tasks
@hedning
Copy link
Contributor

hedning commented Nov 28, 2018

This should probably be targeted at staging-next, as it's a blocker.

matthewbauer added a commit to matthewbauer/nixpkgs that referenced this pull request Nov 28, 2018
@matthewbauer
make-derivation: remove selfConsistent check
13e45ae 
version is set in lots of places but might not need to be in a name.

Alternative to NixOS#50364.
@Synthetica9 Synthetica9 changed the base branch from staging to staging-next November 28, 2018 20:48
@Synthetica9
Copy link
Member Author

@hedning Done 👍

@GrahamcOfBorg GrahamcOfBorg added 8.has: package (new) This PR adds a new package 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 28, 2018
FRidh
FRidh requested changes Nov 30, 2018
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Instead of only pointing in a commit message, actually describe what the commit is about.

@FRidh
Copy link
Member

FRidh commented Nov 30, 2018

I agree here with @matthewbauer. I like it that name is automatically composed of pname and version, but throwing an exception goes too far. As is seen here, version is a perfectly fine parameter and there are cases where one doesn't want to include the version in the name and so just pass in name. In my opinion one should have the freedom to do that and so I would like to see the exception part being removed.

FRidh pushed a commit that referenced this pull request Nov 30, 2018
@matthewbauer @FRidh
make-derivation: remove selfConsistent check
28e2277 
version is set in lots of places but might not need to be in a name.

Alternative to #50364.
@Synthetica9
Copy link
Member Author

I agree here with @matthewbauer. I like it that name is automatically composed of pname and version, but throwing an exception goes too far. As is seen here, version is a perfectly fine parameter and there are cases where one doesn't want to include the version in the name and so just pass in name. In my opinion one should have the freedom to do that and so I would like to see the exception part being removed.

Maybe a nice middle ground would be to trace a warning message, but allow it?

@FRidh
Copy link
Member

FRidh commented Nov 30, 2018 •
edited
Loading

A trace would still imply something is not OK.

@Synthetica9 Synthetica9 closed this Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh

@infinisil infinisil

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Synthetica9 @matthewbauer @hedning @FRidh @GrahamcOfBorg