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

weechatScripts.weechat-otr: init at 1.9.2 #62743

Merged
merged 1 commit into from
Sep 4, 2019

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Jun 5, 2019

Motivation for this change

This PR adds the otr.py script for WeeChat.

As discussed both in the NixOS Wiki and in potr's issue tracker before, potr does not work properly with the current version of pycryptodome and requires an older version of pycrpto. Therefore, the last release of pycrypto, with Debian's security patches applied, will be used.

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Jun 5, 2019
potr = pythonPackages.potr.overridePythonAttrs (oldAttrs: {
propagatedBuildInputs = [ pycrypto ];
});
in stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

bit nitpicky but might look nicer with a line break somewhere round here

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there is a style guide for this. Scrolling through nixpkgs' manual brought up another in stdenv.mkDerivation in the same line. The let example in nix's manual does also continues on the same line.

@oxzi
Copy link
Member Author

oxzi commented Jul 6, 2019

This PR was opened a month ago. What's the status?

@Lassulus
Copy link
Member

Seems to have fallen under the radar. If you could post the usage here I will test this.

@oxzi
Copy link
Member Author

oxzi commented Sep 2, 2019

@Lassulus: Thanks for picking up this PR.

You can test this WeeChat-Script as described in the WeeChat chapter of the nixpkgs manual. All you have to do is create an overlay for WeeChat with this package as a script.

@Lassulus
Copy link
Member

Lassulus commented Sep 4, 2019

alright, tested with the checkout PR and this beauty here:
nix-shell -p '(weechat.override { configure = {...}: { scripts = [ weechatScripts.weechat-otr ]; };})'
seems to work, since weechat shows this at startup:
registered script "otr", version 1.9.2 (Off-the-Record messaging for IRC)

can we build this with python3? python2 is gonna be EOL quite soon

@oxzi
Copy link
Member Author

oxzi commented Sep 4, 2019

@Lassulus: Thanks a lot for testing the changes and your feedback.

can we build this with python3? python2 is gonna be EOL quite soon

Afaik, all WeeChat scripts are using the Python version of the weechat package. This packages inherits pythonPackages, if not overwritten. In the all-packages.nix file there is pythonPackages = python.pkgs and python = python2.

Thus I think that this is not in the scope of this PR. However, with WeeChat switching all scripts to Python 3 for the same deprecation reasons, one should do the same for the nixpkgs. On the other hand, changing python to python3 should do the work for WeeChat - and will likely break a thousand other packages.

@Lassulus
Copy link
Member

Lassulus commented Sep 4, 2019

alright, then I'm gonna merge this.

@Lassulus Lassulus merged commit 85de89d into NixOS:master Sep 4, 2019
@oxzi oxzi deleted the weechat-otr-1.9.2 branch September 4, 2019 11:41
@Ma27
Copy link
Member

Ma27 commented Sep 4, 2019

tbh I haven't tried it yet, but I'd try to build weechat (and some of the scripts we support here) with python3 and if that works I'd simply pin python to python3 for weechat.

@koolfy
Copy link

koolfy commented Nov 25, 2019

Hello, I just wanted to drop by very quickly to publicly state that I am aware of the pycryptodome issue, and very ashamed of it. Work on that front has started out a while ago (proof: python-otr/pure-python-otr#68 ) and I'm the one responsible for the lack of progress :(

Thanks for the hack to make it work, I promise I'm trying to sort it out soon so you don't need to ship pycrypto just for potr anymore :/

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: 0 This PR does not cause any packages to rebuild
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants