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

doc/meta.xml: don't encourage users to add a meta.version attribute #12156

Merged
merged 2 commits into from
Jan 5, 2016

Conversation

peti
Copy link
Member

@peti peti commented Jan 5, 2016

That attribute is completely redundant since it just duplicates information from "name".

Cc: @7c6f434c who added that section in e39b1f4.

That attribute is completely redundant since it just duplicates information
from "name".

Cc: @7c6f434c who added that section in e39b1f4.
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @7c6f434c, @edolstra and @domenkozar to be potential reviewers

@edolstra
Copy link
Member

edolstra commented Jan 5, 2016

👍

@7c6f434c
Copy link
Member

7c6f434c commented Jan 5, 2016

Well, if we slowly replace string parsing with separate explicit specification, at some point we will get rid of the sad stories like the recent node-7f one by being able to specify name and version separately (like many package managers already do).

@domenkozar
Copy link
Member

+1 for making version as required attribute to derivation for Nix as comparing versions is what Nix does as a supported feature.

+1 for this PR as a step to move this way (many derivations already pass version parameter).

peti added a commit that referenced this pull request Jan 5, 2016
…m-manual

doc/meta.xml: don't encourage users to add a meta.version attribute
@peti peti merged commit 5a7116f into NixOS:master Jan 5, 2016
@peti peti deleted the remove-meta-version-attribute-from-manual branch January 5, 2016 16:43
@7c6f434c
Copy link
Member

7c6f434c commented Jan 5, 2016

+1 for making version as required attribute to derivation for Nix as comparing versions is what Nix does as a supported feature.

+1 for this PR as a step to move this way (many derivations already pass version parameter).

Isn't it a step towards not passing version except as a part of
derivation name?

@domenkozar
Copy link
Member

If version was a separate attribute, we wouldn't need to parse it.

@7c6f434c
Copy link
Member

7c6f434c commented Jan 5, 2016

And this PR specifically remove an instance of passing the version in favour of parsing.

@peti
Copy link
Member Author

peti commented Jan 5, 2016

Is there an issue, Wiki page, mailing list posting, or some other document that explains what exactly we'd gain by having version as a separate attribute? I am totally in favor of having that, if it makes sense, but it's not obvious to me yet what that change would allow us to do that we can't do today, and probably this issue is not the right place to discuss that.

@domenkozar
Copy link
Member

We're currently extracting version from name, having a separate attribute for version would eliminate this error prone approach. No code or docs yet, this is just my suggestion for Nix.

@peti
Copy link
Member Author

peti commented Jan 5, 2016

For what it's worth, the commits 7ca8e13 and af8c1f3 make the parsing approach slightly less error-prone.

@nckx
Copy link
Member

nckx commented Jan 5, 2016

What's the argument for smashing version into name, and then having to heuristically sniff it back out every time?

Of course the current duplication is non-ideal, but this PR doesn't do anything to improve the situation.

@peti
Copy link
Member Author

peti commented Jan 5, 2016

@nckx, if you want to improve the situation, please don't hesitate to submit a PR that does!

@nckx
Copy link
Member

nckx commented Jan 5, 2016

@peti: That makes little sense if the attribute is being deprecated.

@bjornfor
Copy link
Contributor

bjornfor commented Jan 5, 2016

@nckx: I think they meant to have drv.version instead of drv.meta.version.

@nckx
Copy link
Member

nckx commented Jan 5, 2016

@bjornfor, that would be grand indeed, and if that's the case I misread it as defending the status quo: whatever the case, ‘completely redundant’ a separate version attribute is very much not.

Sorry @peti — I can be bloody literal-minded sometimes. :-)

@7c6f434c
Copy link
Member

7c6f434c commented Jan 6, 2016

@bjornfor : I see that the commit removed drv.meta.version without adding drv.version. This is moving us further from the result you describe. I think that an optional version specification is more in spirit of meta, making an obligatory version parameter for derivation would also be fine.

Leaving no version attribute and using derivation name parsing for building the source link seems to increase fragility. Especially after #12151. Given that OpenSSL 1.0.2e and 7f are both existing pieces of software on the same planet, derivation name parsing is not likely to be completely safe.

@vcunat
Copy link
Member

vcunat commented Jan 6, 2016

What's the argument for smashing version into name, and then having to heuristically sniff it back out every time?

I know one use case for parsing it – getting name and version just from a path, e.g. $ nix-env -i ./result or $ nix-env -i /nix/store/foo. Of course, we could introduce $out/nix-support/meta.nix or something similar.

@AndersonTorres
Copy link
Member

My two cents:
The version attribute is useful because of parsing problems. There are many softwares and their versioning varies.
It could be made as I am doing for now: putting it as meta.version or something like.

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.

9 participants