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

Jump to bottom

q: init at 1.7.1 #64785

Merged
merged 1 commit into from Sep 2, 2019
Merged

q: init at 1.7.1 #64785

merged 1 commit into from Sep 2, 2019

Conversation

Taneb
Copy link
Contributor

@Taneb Taneb commented Jul 15, 2019

Motivation for this change
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.

@emmanuelrosa
Copy link
Contributor

I tested a few examples on NixOS, and they worked:

[emmanuel@nixpkgs/]$ ps -ef | ./result/bin/q -H "SELECT UID,COUNT(*) cnt FROM - GROUP BY UID ORDER BY cnt DESC LIMIT 3"
root 142
emmanuel 38
dnscryp+ 1
[emmanuel@nixpkgs/]$ find /tmp -ls | ./result/bin/q "SELECT c5,c6,sum(c7)/1024.0/1024 AS total FROM - GROUP BY c5,c6 ORDER BY total desc"
find: ‘/tmp/systemd-private-3c2a8e4e6ebe4b49b4d9ddcdc8ad2b1e-systemd-timesyncd.service-OwVzMS’: Permission denied
find: ‘/tmp/systemd-private-3c2a8e4e6ebe4b49b4d9ddcdc8ad2b1e-rngd.service-gUete3’: Permission denied
find: ‘/tmp/systemd-private-3c2a8e4e6ebe4b49b4d9ddcdc8ad2b1e-accounts-daemon.service-uHyRVq’: Permission denied
find: ‘/tmp/systemd-private-3c2a8e4e6ebe4b49b4d9ddcdc8ad2b1e-systemd-logind.service-LsvPZC’: Permission denied
find: ‘/tmp/systemd-private-3c2a8e4e6ebe4b49b4d9ddcdc8ad2b1e-dnscrypt-proxy.service-vhY35O’: Permission denied
find: ‘/tmp/systemd-private-3c2a8e4e6ebe4b49b4d9ddcdc8ad2b1e-cups.service-gnZDg1’: Permission denied
find: ‘/tmp/systemd-private-3c2a8e4e6ebe4b49b4d9ddcdc8ad2b1e-rtkit-daemon.service-4PoYap’: Permission denied
root root 0.00122547149658
emmanuel users 2.47955322266e-05
[emmanuel@nixpkgs/]$ ps -ef | ./result/bin/q -H "SELECT UID,COUNT(*) cnt FROM - GROUP BY UID ORDER BY cnt DESC LIMIT 3"
root 140
emmanuel 38
dnscryp+ 1

FRidh
FRidh requested changes Jul 22, 2019
View reviewed changes
pkgs/tools/misc/q/default.nix Outdated
{ stdenvNoCC, fetchFromGitHub, python2 }:

stdenvNoCC.mkDerivation rec {
pname = "q";
Copy link
Member

Choose a reason for hiding this comment

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

No single letter names. q-text-as-data instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where is this policy stated? There seemt o be other packages (eg. j) with single letter names.

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @FRidh

Copy link
Member

Choose a reason for hiding this comment

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

j was added very long ago, when Nixpkgs was also still tiny by comparison.
This policy has not been documented and falls in my opinion under common sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is also t, added in more recently in 2015.

I can change the name but I do feel like it would make it harder to find the package: when I first checked if q had already been package I did not check q-text-as-data or similar.

Copy link
Member

Choose a reason for hiding this comment

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

Considering it is a single letter it is in general hard to find. At least with q-text-as-data it's in line with other distro's.

pkgs/top-level/all-packages.nix Outdated
@@ -5512,6 +5512,8 @@ in

ocz-ssd-guru = callPackage ../tools/misc/ocz-ssd-guru { };

q = callPackage ../tools/misc/q { };
Copy link
Member

Choose a reason for hiding this comment

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

No single letter attributes. q-text-as-data instead.

@aanderse
Copy link
Member

@Taneb are you able to continue with this? This looks like a pretty fun piece of software I want to make an excuse to try... so shall we get this merged in time for 19.09? 😄

@Taneb
Copy link
Contributor Author

Taneb commented Aug 31, 2019

Oh yes! Honestly this had completely slipped my mind. I'll go ahead and change the name...

@aanderse
Copy link
Member

Oh yes! Honestly this had completely slipped my mind. I'll go ahead and change the name...

Great! I look forward to you pushing the commit. After that I see no reason not to merge, just in time for 19.09!

@aanderse
Copy link
Member

aanderse commented Sep 1, 2019

@Taneb it seems like @FRidh wanted you to change the value of pname as well, if I'm not mistaken.

@FRidh @srhb after the change to pname are you happy to merge?

@Taneb
Copy link
Contributor Author

Taneb commented Sep 2, 2019

@FRidh can you re-review, please?

@FRidh FRidh merged commit ddefb5f into NixOS:master Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srhb srhb srhb left review comments

@FRidh FRidh FRidh requested changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Taneb @emmanuelrosa @aanderse @srhb @FRidh