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-commander: init at unstable-2021-04-18 #119895

Merged
merged 5 commits into from Apr 30, 2021

Conversation

seb314
Copy link
Contributor

@seb314 seb314 commented Apr 19, 2021

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/) (tested the python script in bin/matrix-commander, sending messages works)
  • 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.

@r-rmcgibbo
Copy link

r-rmcgibbo commented Apr 19, 2021

Result of nixpkgs-review pr 119895 at c26e0ed run on aarch64-linux 1

1 package built successfully:
  • matrix-commander
2 suggestions:
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/applications/networking/instant-messengers/matrix-commander/default.nix:23:3:

       |
    23 |   installPhase = ''
       |   ^
    
  • warning: unclear-gpl

    gpl3 is a deprecated license, please check if project uses gpl3Plus or gpl3Only and change meta.license accordingly.

    Near pkgs/applications/networking/instant-messengers/matrix-commander/default.nix:32:5:

       |
    32 |     license = lib.licenses.gpl3;
       |     ^
    

Result of nixpkgs-review pr 119895 at c26e0ed run on x86_64-linux 1

1 package built successfully:
  • matrix-commander
2 suggestions:
  • warning: unclear-gpl

    gpl3 is a deprecated license, please check if project uses gpl3Plus or gpl3Only and change meta.license accordingly.

    Near pkgs/applications/networking/instant-messengers/matrix-commander/default.nix:32:5:

       |
    32 |     license = lib.licenses.gpl3;
       |     ^
    
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/applications/networking/instant-messengers/matrix-commander/default.nix:23:3:

       |
    23 |   installPhase = ''
       |   ^
    

@Mindavi
Copy link
Contributor

Mindavi commented Apr 19, 2021

LGTM. Builds on my machine and runs (I haven't actually tried connecting to a matrix server though).

{ pkgs, fetchFromGitHub, lib }:

pkgs.stdenv.mkDerivation {
name = "matrix-commander";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name = "matrix-commander";
pname = "matrix-commander";
version = "unstable-XXXX-XX-XX"

replace XXXX-XX-XX with the date of the commit.

Comment on lines 13 to 21
propagatedBuildInputs = [
pkgs.cacert ] ++
(with pkgs.python38Packages; [
matrix-nio
magic
markdown
pillow
urllib3
]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
propagatedBuildInputs = [
pkgs.cacert ] ++
(with pkgs.python38Packages; [
matrix-nio
magic
markdown
pillow
urllib3
]);
propagatedBuildInputs = [ cacert ]
++ (with python3Packages; [
matrix-nio
magic
markdown
pillow
urllib3
]);

@@ -0,0 +1,36 @@
{ pkgs, fetchFromGitHub, lib }:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ pkgs, fetchFromGitHub, lib }:
{ lib, fetchFromGitHub, cacert, python3Packages }:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also added stdenv in order to get mkDerivation -- or is there a better way to do that?

Comment on lines 29 to 34
meta = {
description = "Simple but convenient CLI-based Matrix client app for sending and receiving";
homepage = "https://github.com/8go/matrix-commander";
license = lib.licenses.gpl3;
platforms = lib.platforms.linux;
maintainers = [ lib.maintainers.seb314 ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
meta = {
description = "Simple but convenient CLI-based Matrix client app for sending and receiving";
homepage = "https://github.com/8go/matrix-commander";
license = lib.licenses.gpl3;
platforms = lib.platforms.linux;
maintainers = [ lib.maintainers.seb314 ];
meta = with lib; {
description = "Simple but convenient CLI-based Matrix client app for sending and receiving";
homepage = "https://github.com/8go/matrix-commander";
license = licenses.gpl3;
platforms = platforms.linux;
maintainers = [ maintainers.seb314 ];

gpl3Only or gpl3Plus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -23946,6 +23946,8 @@ in
canonicaljson;
};

matrix-commander = callPackage ../applications/networking/instant-messengers/matrix-commander {};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
matrix-commander = callPackage ../applications/networking/instant-messengers/matrix-commander {};
matrix-commander = callPackage ../applications/networking/instant-messengers/matrix-commander { };

apply most SuperSandro2000's review comments;

used python3.withPackages -- this might be the correct
way to get runtime python dependencies to work even
when matrix-commander is a dependency of another packages
(as opposed to testing in nix-shell). I'm not sure though.

aiofiles seemed to be a missing runtime dependency when called
from another (nix-packaged, jvm-based) app -- there was an
import error without it.

cacerts might require explicit pinning, as described here:
https://gist.github.com/CMCDragonkai/1ae4f4b5edeb021ca7bb1d271caca999
(relevant snippet: wrapProgram $out/bin/program \
  --set SSL_CERT_FILE "${cacert}/etc/ssl/certs/ca-bundle.crt"
  )
@seb314
Copy link
Contributor Author

seb314 commented Apr 25, 2021

thanks for the review! In addition to applying most of your suggestions (see comments), I did some changes that I hope will be more correct regarding runtime dependencies (it seems that while the original commit worked fine from nix-shell, it resulted in an import error when I tried using matrix commander as a dependency from another nix-packaged app (jvm-based). The new version works locally from nix shell (as before) and avoids the import error when called from the other program. Hovewer, when called from my other app, actuall sending still doesn't work -- but that might be a problem on the other program/package's side...

meta = with lib; {
description = "Simple but convenient CLI-based Matrix client app for sending and receiving";
homepage = "https://github.com/8go/matrix-commander";
license = licenses.gpl3;
Copy link
Member

Choose a reason for hiding this comment

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

gpl3Plus or gpl3Only?

Copy link
Contributor Author

@seb314 seb314 Apr 28, 2021

Choose a reason for hiding this comment

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

I didn't find any mentions about that in the matrix-commander repo, so I asked on github, but there's no answer yet.

Should we wait, infer gpl3Only, or keep gpl3 for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think keeping it gpl3 for now and adding a link to the issue is the best way to handle this. Then it's easy to change in the future if / when an answer comes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, added a note in 1331ee3

@SuperSandro2000 SuperSandro2000 changed the title matrix-commander: init matrix-commander: init at unstable-2021-04-18 Apr 29, 2021
@SuperSandro2000 SuperSandro2000 merged commit 3f2f7d3 into NixOS:master Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants