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

scli: init at 0.6.1 #122315

Merged
merged 1 commit into from May 29, 2021
Merged

scli: init at 0.6.1 #122315

merged 1 commit into from May 29, 2021

Conversation

alexeyre
Copy link
Contributor

@alexeyre alexeyre commented May 9, 2021

Motivation for this change

Response to #120615

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/)
  • 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.
Improvements To-Do
  • Find a way to implement a useful checkPhase

@alexeyre
Copy link
Contributor Author

alexeyre commented May 9, 2021

Result of nixpkgs-review pr 122315 run on x86_64-darwin 1

1 package built:
  • scli

Copy link
Contributor

@Pacman99 Pacman99 left a comment

Choose a reason for hiding this comment

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

This looks like a cool program! Overall the package looks great, thanks for submitting it.

I would also recommend running nixpkgs-fmt on the package's file to standardize the formatting.

};
propagatedBuildInputs = [ signal-cli urwid urwid-readline dbus ];
dontBuild = true;
doCheck = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to include a checkPhase of just running scli --help or something similar to ensure it works. Maybe you could test its ability to pickup signal-cli too, once you get that to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately scli tries to create a ~/.local/share/scli directory upon initial run, which nix freaks out about:
https://gist.github.com/alex-eyre/dd3a0a8d8c132715725d703bd9841dac
I don't know if there's anything we can do about that :/

Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably do something like this:

XDG_DATA_HOME=$(mktemp -d)
XDG_CONFIG_HOME=$(mktemp -d)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the help! I've implemented a really basic invocation test in the checkPhase, but I'm not sure what more testing we can do without somehow logging into signal 🤔
12e50ce0117902c60eb880aacc672de6e23d6b25

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/scli/default.nix Show resolved Hide resolved
@alexeyre alexeyre marked this pull request as ready for review May 9, 2021 21:47
@SuperSandro2000 SuperSandro2000 changed the title Init scli at 0.6.0 scli: init at 0.6.0 May 10, 2021
@alexeyre
Copy link
Contributor Author

Result of nixpkgs-review pr 122315 run on x86_64-darwin 1

1 package built:
  • scli

@alexeyre alexeyre changed the title scli: init at 0.6.0 scli: init at 0.6.1 May 13, 2021
@alexeyre alexeyre requested review from SuperSandro2000 and Pacman99 and removed request for SuperSandro2000 and Pacman99 May 14, 2021 14:51
@alexeyre
Copy link
Contributor Author

alexeyre commented May 14, 2021

This actually requires zkgroup for full functionality, which according to docs should be embedded into signal-cli itself, so I'll have a look at doing that.
Otherwise, it's having a hard time finding libsignal-client, which as far as I'm aware should be installed with at least one of signal-cli or libsignal-protocol-c, but it can't find it even when both are supplied in propagatedBuildInputs. I'll do some more digging, but this is not ready to be merged yet.

cc @ivan @erictapen

pkgs/applications/misc/scli/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/scli/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/scli/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/scli/default.nix Outdated Show resolved Hide resolved

installPhase = ''
mkdir -p $out/bin
install -m755 -D scli $out/bin/scli
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 20bb6ae

scli: add license

scli: add basic checkPhase

scli: apply formatting changes from @SuperSandro2000

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

scli: update src sha256

scli: remove chmod from checkPhase

scli: add pathShebangs
@SuperSandro2000 SuperSandro2000 merged commit aeb4b89 into NixOS:master May 29, 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

3 participants