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

nixos-version: use version_ instead of version #50364

Closed

Conversation

Synthetica9
Copy link
Member

@Synthetica9 Synthetica9 commented Nov 14, 2018

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.

@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)

@matthewbauer
Copy link
Member

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

@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
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 👍

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
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

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants