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

xzoom: fix version #72178

Merged
merged 1 commit into from
Nov 17, 2019
Merged

xzoom: fix version #72178

merged 1 commit into from
Nov 17, 2019

Conversation

davidak
Copy link
Member

@davidak davidak commented Oct 28, 2019

Motivation for this change

After #71528 the error disappeared, but we have a different version than everyone else.

That is because we had the patch version applied to the version. The patch comes from debian, but even they use the official release version. So we should too.

https://repology.org/project/xzoom/versions

Screenshot from 2019-10-28 17-34-18

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 nix-review --run "nix-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @7c6f434c

@ofborg ofborg bot requested a review from 7c6f434c October 28, 2019 16:50
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Oct 28, 2019
@worldofpeace
Copy link
Contributor

I believe there's the .24 because of the patch? I do notice exherbo has p24 which I guess means patch 24. It probably should be in the version string to signify that it's in fact different than 0.34 because of the patch.

@davidak
Copy link
Member Author

davidak commented Oct 29, 2019

I believe there's the .24 because of the patch?

yes, it's the version of the debian patch. debian itself still uses the upstream version "0.3"

I do notice exherbo has p24 which I guess means patch 24.

yes, i think that's the case

It probably should be in the version string to signify that it's in fact different

maybe, maybe not. it should at least be consistent. i think we don't have a policy for that currently.

but at the same time we have many distro specific patches and still use the official release versions. so i proposed to do it here also...

Do you know a good version scheme to have additional external patches included?

"0.3.24" would be very bad. it implies that its an official version number. and what if that official version get's released?

What if there are multiple patches? bash for example has 23 patches! https://github.com/NixOS/nixpkgs/blob/92a047a6c4d46a222e9c323ea85882d0a7a13af8/pkgs/shells/bash/bash-4.4-patches.nix

@worldofpeace
Copy link
Contributor

I believe there's the .24 because of the patch?

yes, it's the version of the debian patch. debian itself still uses the upstream version "0.3"

I do notice exherbo has p24 which I guess means patch 24.

yes, i think that's the case

It probably should be in the version string to signify that it's in fact different

maybe, maybe not. it should at least be consistent. i think we don't have a policy for that currently.

but at the same time we have many distro specific patches and still use the official release versions. so i proposed to do it here also...

Do you know a good version scheme to have additional external patches included?

"0.3.24" would be very bad. it implies that its an official version number. and what if that official version get's released?

What if there are multiple patches? bash for example has 23 patches! https://github.com/NixOS/nixpkgs/blob/92a047a6c4d46a222e9c323ea85882d0a7a13af8/pkgs/shells/bash/bash-4.4-patches.nix

Well with the case with debian, it's probably a good idea to believe 0.3 != 0.3 because debian is pretty much downstream patches all the way down 😄

But right, 0.3.24 is not good at all. Not sure why it's like that in debian, I think they'd do something like 0.3-debian_revision

Perhaps repology stripped that info.

@davidak
Copy link
Member Author

davidak commented Nov 7, 2019

Not sure why it's like that in debian, I think they'd do something like 0.3-debian_revision
Perhaps repology stripped that info.

you are right!

https://packages.debian.org/unstable/source/xzoom

xzoom (0.3-25)

I'm not sure if we need to show if our packages have patches or not. Patches should in best case fix issues and provide the behavior upstream intended. If something is broken with out package, users will create an issue here. We will not add new features with patches! But sometimes fix security issues where upstream has not released a new version yet.

We have to discuss it somewhere. What would be a good place? nixpkgs issue, nix issue, discourse, RFC?

@davidak
Copy link
Member Author

davidak commented Nov 13, 2019

The discussion in #73228 leads to the conclusion that it don't really makes sense to add patch version to package version.

Copy link
Member

@Lassulus Lassulus left a comment

Choose a reason for hiding this comment

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

tested with nix-review, changes look good

@Lassulus Lassulus merged commit bc1a409 into NixOS:master Nov 17, 2019
@davidak davidak deleted the xzoom-version branch November 18, 2019 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants