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

matrix-appservice-irc 0.26.1 -> 0.30.0 #137035

Closed
wants to merge 2 commits into from

Conversation

DavHau
Copy link
Member

@DavHau DavHau commented Sep 7, 2021

Motivation for this change

Hackint matrix irc bridge broke because this package is outdated

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@DavHau DavHau force-pushed the update-matrix-appservice-irc branch from affe18e to db2264b Compare September 7, 2021 22:00
@andir
Copy link
Member

andir commented Sep 7, 2021

@ofborg test matrix-appservice-irc

@DavHau
Copy link
Member Author

DavHau commented Sep 7, 2021

EditorConfig check fails because the REVISION file doesn't end with a newline. But this is done on purpose since the version should not contain a new line. Is there a way to make an exception for the test?

@mweinelt

This comment has been minimized.

@@ -1,16 +1,23 @@
{ pkgs, nodePackages, makeWrapper, nixosTests, nodejs, stdenv, lib, ... }:
{ pkgs, nodePackages, makeWrapper, nixosTests, nodejs, stdenv, lib, fetchFromGitHub, ... }:
Copy link
Member

Choose a reason for hiding this comment

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

We should drop the ... here. It isn't required and not good practice. I know this isn't your fault but if we change it we can make a slight improvement.

@mweinelt
Copy link
Member

mweinelt commented Sep 7, 2021

diff --git a/nixos/tests/matrix-appservice-irc.nix b/nixos/tests/matrix-appservice-irc.nix
index 79b07ef83c5..264142a9066 100644
--- a/nixos/tests/matrix-appservice-irc.nix
+++ b/nixos/tests/matrix-appservice-irc.nix
@@ -131,6 +131,8 @@ import ./make-test-python.nix ({ pkgs, ... }:
     };
 
     testScript = ''
+      import pathlib
+
       start_all()
 
       ircd.wait_for_unit("ngircd.service")

rm -f package.json package-lock.json
wget https://github.com/matrix-org/matrix-appservice-irc/raw/$TARGET_VERSION/package.json
wget -O package-lock-temp.json https://github.com/matrix-org/matrix-appservice-irc/raw/$TARGET_VERSION/package-lock.json
echo -n "$TARGET_VERSION" > ./REVISION
Copy link
Member

Choose a reason for hiding this comment

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

Just give it a newline and trim / get the first line in the nix revision. That makes this part more robust to manual updates & gets rid of the editorconfig error.

@DavHau
Copy link
Member Author

DavHau commented Sep 7, 2021

nixos/tests/matrix-appservice-irc.nix

Thanks. I overlooked the test.
After fixing the import, still erroring out with:

client # Traceback (most recent call last):
client #   File "/run/current-system/sw/bin/do_test", line 7, in <module>
client #     matrix.register_with_password(username="alice", password="foobar")
client #   File "/nix/store/630jdrv3dxh67gvzd3ca9xmc2cn7an4w-python3-3.9.6-env/lib/python3.9/site-packages/matrix_client/client.py", line 203, in register_with_password
client #     response = self.api.register(
client #   File "/nix/store/630jdrv3dxh67gvzd3ca9xmc2cn7an4w-python3-3.9.6-env/lib/python3.9/site-packages/matrix_client/api.py", line 160, in register
client #     return self._send(
client #   File "/nix/store/630jdrv3dxh67gvzd3ca9xmc2cn7an4w-python3-3.9.6-env/lib/python3.9/site-packages/matrix_client/api.py", line 748, in _send
client #     raise MatrixRequestError(
client # matrix_client.errors.MatrixRequestError: 400: {"errcode":"M_UNKNOWN","error":"An access token should not be provided on requests to /register (except if type is m.login.application_service)"}

For me it looks like a bug in the matrix_client python package.
Our code is very simple and we do definitely not pass any access token:

import socket
from matrix_client.client import MatrixClient
from time import sleep

matrix = MatrixClient("${homeserverUrl}")
matrix.register_with_password(username="alice", password="foobar")  // <-- it fails here
...

@r-rmcgibbo
Copy link

r-rmcgibbo commented Sep 7, 2021

Result of nixpkgs-review pr 137035 at db2264b run on x86_64-linux 1

2 packages built successfully:
  • matrix-appservice-irc
  • nixos-install-tools
3 suggestions:
  • warning: missing-phase-hooks

    buildPhase should probably contain runHook preBuild and runHook postBuild.

    Near pkgs/development/node-packages/node-env.nix:410:14:

        |
    410 |       inherit dontNpmInstall preRebuild unpackPhase buildPhase;
        |              ^
    
  • warning: build-tools-in-build-inputs

    util-linux is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

    Near pkgs/development/node-packages/node-env.nix:402:7:

        |
    402 |       buildInputs = [ tarWrapper python nodejs ]
        |       ^
    
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall.

    Near pkgs/development/node-packages/node-env.nix:417:7:

        |
    417 |       installPhase = ''
        |       ^
    

Result of nixpkgs-review pr 137035 at db2264b run on aarch64-linux 1

2 packages built successfully:
  • matrix-appservice-irc
  • nixos-install-tools

@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Sep 10, 2021
@@ -0,0 +1 @@
0.30.0
Copy link
Member

Choose a reason for hiding this comment

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

Nix can return this value from the derivation eg nix-instantiate --eval -E 'with (import ./. { }).pkgs; python3Packages.pyopenssl.version'

@piegamesde
Copy link
Member

I've opened an upstream issue for the test. In the meantime, downgrading to a lower version of the library should work.

@andir
Copy link
Member

andir commented Sep 17, 2021

I've opened an upstream issue for the test. In the meantime, downgrading to a lower version of the library should work.

Any reason we aren't just using nio instead? The python sdk even recommends that: https://github.com/matrix-org/matrix-python-sdk#project-status

@piegamesde
Copy link
Member

This is purely historical (maybe due to cargo cult, maybe it was the best choice at the time); switching to a different library should be purely a matter of somebody taking the time and doing the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants