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

weechat: build with python3 by default #68110

Merged
merged 2 commits into from
Sep 5, 2019
Merged

weechat: build with python3 by default #68110

merged 2 commits into from
Sep 5, 2019
+30 −78

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Sep 4, 2019 •
edited
Loading

Motivation for this change

WeeChat also supports Python3 for scripts which should be preferred as
CPython2 is about to get EOLed soon: https://weechat.org/scripts/python3/

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 @

Sorry, something went wrong.

jonringer
jonringer reviewed Sep 5, 2019
View reviewed changes
pkgs/top-level/all-packages.nix Outdated
};

weechat = wrapWeechat weechat-unwrapped { };

weechatScripts = dontRecurseIntoAttrs (callPackage ../applications/networking/irc/weechat/scripts { });
weechatScripts = dontRecurseIntoAttrs (callPackage ../applications/networking/irc/weechat/scripts {
Copy link
Contributor

Choose a reason for hiding this comment

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

dontRecurseIntoAttrs is now the default, I think the implementation now is just the id function drv: drv

Suggested change
weechatScripts = dontRecurseIntoAttrs (callPackage ../applications/networking/irc/weechat/scripts {
weechatScripts = callPackage ../applications/networking/irc/weechat/scripts {

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, had to check

nixpkgs/pkgs/top-level/all-packages.nix

Line 75 in 5e86891

dontRecurseIntoAttrs = x: x;

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact it's always been the default, dontRecurseIntoAttrs mainly serves a documentation purpose. To quote the comment above the function:

  # This is intended to be the reverse of recurseIntoAttrs, as it is
  # defined now it exists mainly for documentation purposes, but you
  # can also override this with recurseIntoAttrs to recurseInto all
  # the Attrs which is useful for testing massive changes. Ideally,
  # every package subset not marked with recurseIntoAttrs should be
  # marked with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, if it has a purpose, then I'm fine with leaving it in :)

@Mic92
Copy link
Member

Mic92 commented Sep 5, 2019

I agree we should have that at some point. The only thing that concerns me a bit that even archlinux, who were early python3 adopters, build weechat with python2: https://git.archlinux.org/svntogit/community.git/tree/trunk/PKGBUILD?h=packages/weechat#n13
On the other hand it seems that most of the python2-only scripts seems quite old: https://weechat.org/scripts/stable/language/python2-only/
Personally I don't use any of plugins, but would be interested how other weechat users think about this.

@Ma27
Copy link
Member Author

Ma27 commented Sep 5, 2019

I agree we should have that at some point. The only thing that concerns me a bit that even archlinux, who were early python3 adopters, build weechat with python2

Last time I checked (about a month ago) they were building a lot of applications with python2, but I'm no arch user, so could've been a false impression.

On the other hand it seems that most of the python2-only scripts seems quite old: https://weechat.org/scripts/stable/language/python2-only/

After taking a look at the list, I didn't find any of our currently supported plugins in there. However I guess I should check all of the python stuff manually (just checked wee-slack and looked if any of our plugins require python2).

@Ma27
Copy link
Member Author

Ma27 commented Sep 5, 2019

So, short update: rechecked all scripts and it seems as weechat-xmpp requires a xmpppy with Python3 support, currently investigating if that's possible.

After that we'd need somebody with more darwin experience to fix the build, after that this should be ready IMHO.

@Ma27
Copy link
Member Author

Ma27 commented Sep 5, 2019

Rebased onto current master, also dropped weechat-xmpp as it relies on pretty old dependencies and appears rather unmaintained atm.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation labels Sep 5, 2019
@jonringer
Copy link
Contributor

There's also a reference to the weechat-xmpp in aliases

attribute 'weechat-xmpp' missing, at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/eval-2-shlevy.ewr1.nix.ci/pkgs/top-level/aliases.nix:383:18

@Ma27
Copy link
Member Author

Ma27 commented Sep 5, 2019

Ahh, right, fixed! :)

Ma27 added 2 commits September 5, 2019 20:03
@Ma27
weechat: build with python3 by default
689a50f 
WeeChat also supports Python3 for scripts which should be preferred as
CPython2 is about to get EOLed soon: https://weechat.org/scripts/python3/
@Ma27
weechatScripts.weechat-xmpp: remove
e4bc0e2 
This plugin is fairly outdated and depends on python2 libraries that
don't receive any updates either (xmpppy for instance[1]).

[1] https://pypi.org/project/xmpppy/
@lheckemann lheckemann merged commit 734da72 into NixOS:master Sep 5, 2019
@Ma27 Ma27 deleted the weechat-python3 branch September 5, 2019 18:09
Mic92
Mic92 reviewed Sep 6, 2019
View reviewed changes
pkgs/top-level/aliases.nix
@@ -380,7 +380,6 @@ mapAliases ({
virtviewer = virt-viewer; # added 2015-12-24
vorbisTools = vorbis-tools; # added 2016-01-26
webkit = webkitgtk; # added 2019-03-05
weechat-xmpp = weechatScripts.weechat-xmpp; # added 2018-09-06
Copy link
Member

Choose a reason for hiding this comment

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

Maybe?

weechat-xmpp = throw "weechat-xmpp has been removed as it doesn't receive
     any updates from upstream and depends on outdated Python2-based modules.";

Copy link
Member

Choose a reason for hiding this comment

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

Not in favour. Removal already throws an error, and the reason for removal is documented in the release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92

@jonringer jonringer

@the-kenny the-kenny

@lovek323 lovek323

@lheckemann lheckemann

@Lassulus Lassulus

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Ma27 @Mic92 @jonringer @lheckemann @bqv