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.

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

vit 2.0.0 #71197

Merged
merged 2 commits into from Oct 16, 2019
Merged

vit 2.0.0 #71197

merged 2 commits into from Oct 16, 2019

Conversation

arcnmx
Copy link
Member

@arcnmx arcnmx commented Oct 15, 2019 •
edited

Motivation for this change

https://groups.google.com/forum/#!topic/taskwarrior-user/0dTbvFOOZY0

I'm not sure whether I should remove the previous vit package and replace the attr with this, or keep it around as vit1, or?

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 @dtzWill as maintainer of the existing vit 1.3 package, also do you want to be on this version too?

@arcnmx arcnmx requested a review from FRidh as a code owner October 15, 2019 17:19
@jonringer
Copy link
Contributor

@worldofpeace thoughts on deprecation of pythonPackages.vit? It's currently marked broken.

@arcnmx
Copy link
Member Author

arcnmx commented Oct 16, 2019

deprecation of pythonPackages.vit

I'm not sure I follow, what does deprecation mean in the context of nixpkgs? I included meta.broken = python.isPy2 to indicate that it only supports python 3, though I'm not sure if that's the canonical way to do so. versionAtLeast python.version "3.5" would also be a more precise way to express the version requirement.

To clarify the situation...

  • The previous 1.3 perl package is pkgs.vit, and works fine though is a now abandaned branch.
  • This PR introduces the 2.0 release as python3Packages.vit (and pythonPackages.vit though python 2 is unsupported)
  • I'm not sure which decision to make for the top-level attr:
    1. Replace pkgs.vit and remove the old package.
    2. Introduce this as pkgs.vit and rename the old package to pkgs.vit1 or vit1_3?
    3. Introduce this as pkgs.vit2 and retain the previous one for now, or mark it deprecated, or...

@worldofpeace
Copy link
Contributor

So vit 1.3 is in perl, but the latest is rewritten in python.

If vit has moved on from perl this is just a version update.

I see it as they're the same package, you can just update the misc/vit expression in place.
What versions of python it supports is just a detail of the new python code.

worldofpeace
worldofpeace reviewed Oct 16, 2019
View reviewed changes
pkgs/top-level/python-packages.nix Outdated
@@ -5465,6 +5467,8 @@ in {

virtualenvwrapper = callPackage ../development/python-modules/virtualenvwrapper { };

vit = callPackage ../development/python-modules/vit { };
Copy link
Contributor

Choose a reason for hiding this comment

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

is there documented use of it as a python module?

Copy link
Member Author

Choose a reason for hiding this comment

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

Though it sounds like it may a goal for the future, I doubt there's any point now and can just move it all into misc/vit instead.

@jonringer
Copy link
Contributor

I included meta.broken = python.isPy2 to indicate that it only supports python 3

you should use disabled = isPy27 instead

@arcnmx arcnmx force-pushed the pr-vit2 branch 2 times, most recently from 301e43d to 8f190ba Compare October 16, 2019 17:12
@ofborg ofborg bot requested a review from dtzWill October 16, 2019 17:26
worldofpeace
worldofpeace reviewed Oct 16, 2019
View reviewed changes
pkgs/applications/misc/vit/default.nix Outdated
postInstall = ''
wrapProgram $out/bin/vit --prefix PERL5LIB : $PERL5LIB
'';
makeWrapperArgs = [ "--prefix" "PATH" ":" "${taskwarrior}/bin" ];
Copy link
Contributor

Choose a reason for hiding this comment

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

please --suffix so it can be overriden in PATH

@worldofpeace worldofpeace merged commit b28a757 into NixOS:master Oct 16, 2019
@worldofpeace
Copy link
Contributor

Thanks @arcnmx ✨

@arcnmx arcnmx deleted the pr-vit2 branch October 16, 2019 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@worldofpeace worldofpeace worldofpeace left review comments

@FRidh FRidh Awaiting requested review from FRidh

@dtzWill dtzWill Awaiting requested review from dtzWill

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 0 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@arcnmx @jonringer @worldofpeace