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

Avoid package attributes starting with a digit #33897

Merged
merged 2 commits into from Feb 2, 2018
Merged

Conversation

rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Jan 15, 2018

Motivation for this change

See the release notes.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS)
  • Built NixOS manual
  • 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
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@grahamc
Copy link
Member

grahamc commented Jan 15, 2018

I like the intention, but don't like the _. Just a $0.02.

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jan 15, 2018

What do you propose? I went with an underscore because there are already two packages doing this in nixpkgs.

@Mic92
Copy link
Member

Mic92 commented Jan 16, 2018

Please also add renamed packages to pkgs/top-level/aliases.nix

@vcunat
Copy link
Member

vcunat commented Jan 16, 2018

I wonder what's the precedent, in major packaging systems. Have you looked?

Nitpick: you forgot 2048-in-terminal.

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jan 16, 2018

I wonder what's the precedent, in major packaging systems. Have you looked?

Not yet, I will research a bit.

Nitpick: you forgot 2048-in-terminal.

Ah, right. I forgot to link it: 2048-in-terminal is addressed in #33846

@orivej orivej requested a review from edolstra January 16, 2018 11:20
@orivej
Copy link
Contributor

orivej commented Jan 16, 2018

Other package managers do not avoid names starting with digits:

It seems that this issue is better addressed in Nix rather than in Nixpkgs: [ 2bwm ] should not be parsed as [ 2 bwm ].

@vcunat
Copy link
Member

vcunat commented Jan 16, 2018

Hmm, changing nix for this is an interesting option. Still, my eye might tend to parse "389-ds" as subtraction...

@orivej
Copy link
Contributor

orivej commented Jan 16, 2018

The eye will adjust. Nix supports - in attribute names, and I don't think that a-4-b (an ID) should be parsed differently from 4-a-b (currently a subtraction of a-b from 4).

@vcunat
Copy link
Member

vcunat commented Jan 16, 2018

It would be quite an incompatible change. When I remember how the "x:x" change goes...

@orivej
Copy link
Contributor

orivej commented Jan 16, 2018

I have checked that modifying the lexer to support IDs starting with an integer (and prioritizing INTs to IDs to disambiguate) does not affect parsing of any .nix file in Nixpkgs master. I am going to make a PR with this change (after adding tests). WRT this PR, I'd prefer that names in all-packages.nix continue to begin with digits; underscored aliases may be added to aliases.nix.

In 0b16704#r14008920 @edolstra said:

Please don't start package names with a digit. It confuses nix-env and requires quoting of attributes.

But e.g. nix-env -iA 2048-in-terminal works, and the proposed change will obsolete quoting. (nix-env -i 2048-in-terminal works too.)

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jan 16, 2018

@orivej I didn't consider changing Nix syntax. I'm not sure about it though.

@dtzWill
Copy link
Member

dtzWill commented Jan 16, 2018

(however this gets resolved, please include 2048-in-terminal here as well, since it should be handled the same as others in this PR. Thanks!)

@Mic92 Mic92 merged commit a417040 into NixOS:master Feb 2, 2018
@rnhmjoj rnhmjoj deleted the digits branch February 23, 2019 09:36
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

7 participants