Skip to content

Comments

lnav: add prql support#349284

Merged
Aleksanaa merged 4 commits intoNixOS:masterfrom
pcasaretto:lnav-prql
Dec 12, 2024
Merged

lnav: add prql support#349284
Aleksanaa merged 4 commits intoNixOS:masterfrom
pcasaretto:lnav-prql

Conversation

@pcasaretto
Copy link
Contributor

Added PRQL support for lnav.
PRQL support was introduced recently and for recent builds you would get a

✘ error: PRQL is not supported in this build

when you tried querying.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@pcasaretto
Copy link
Contributor Author

Stuck at

This PR does not cleanly list package outputs after merging.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 10, 2024
@Atemu
Copy link
Member

Atemu commented Nov 22, 2024

@ofborg eval

You need to rebase btw., the package has been moved to by-name.

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Nov 28, 2024
@pcasaretto
Copy link
Contributor Author

Thanks for the support Atemu!

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

I'm not super familiar with cargo packaging but the diff LGTM.

@Atemu
Copy link
Member

Atemu commented Nov 28, 2024

How would I verify whether prql support works? I don't know what that is.

@Atemu
Copy link
Member

Atemu commented Nov 28, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 349284


x86_64-linux

✅ 2 packages built:
  • lnav
  • lnav.debug

@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. label Nov 29, 2024
@pcasaretto
Copy link
Contributor Author

You can execute a PRQL query using the existing database prompt (press ;). A query is interpreted as PRQL if it starts with the from keyword. After from, the database table should be provided. The table for the focused log message will be suggested by default. You can accept the suggestion by pressing TAB. To add a new stage to the pipeline, enter a pipe symbol (|), followed by a PRQL transform and its arguments. In addition to the standard set of transforms, lnav provides some convenience transforms in the stats and utils namespaces. For example, stats.count_by can be passed one or more column names to group by and count, with the result sorted by most to least.

https://lnav.org/2024/03/29/prql-support.html

In a version with no support you get

✘ error: PRQL is not supported in this build

Copy link
Member

@symphorien symphorien left a comment

Choose a reason for hiding this comment

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

Fails to build

error: builder for '/nix/store/1ji2immm5l9mfn19fr4y04qfq01yds6c-cargo-deps-vendor-staging.drv' failed with exit code 1;
       last 2 log lines:
       > Running phase: unpackPhase
       > variable $src or $srcs should point to the source

@pcasaretto
Copy link
Contributor Author

Fixed.

@github-actions github-actions bot removed 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Dec 2, 2024
@github-actions github-actions bot added the 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. label Dec 4, 2024
@symphorien
Copy link
Member

Does bumping the sdk mean that we cannot backport this PR to 24.11 as it's a breaking change (and we should only backport the commit making gpm optional)?

@wegank wegank removed 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Dec 5, 2024
@Atemu
Copy link
Member

Atemu commented Dec 5, 2024

I don't think we should backport this in any case.

@Aleksanaa
Copy link
Member

@ofborg build lnav

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. labels Dec 5, 2024
@Atemu
Copy link
Member

Atemu commented Dec 5, 2024

x86_64-darwin appears to still be broken. Could you just test this locally via pkgsx86_64Darwin until it works?

@Aleksanaa
Copy link
Member

I guess it's clang 19 problem so it should fail on aarch64-darwin too.

@ofalvai
Copy link
Contributor

ofalvai commented Dec 10, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 349284


x86_64-darwin

❌ 1 package failed to build:
  • lnav

aarch64-darwin

✅ 1 package built:
  • lnav

@ofalvai
Copy link
Contributor

ofalvai commented Dec 10, 2024

I just ran into the issue of lnav being broken on aarch64-darwin and found this PR.

As you can see above, aarch64-darwin works with this PR, but x86_64 fails with this error:

lnav-x86_64-darwin> readline_curses.cc:847:9: error: use of undeclared identifier 'ptsname_r'; did you mean 'ttyname_r'?
lnav-x86_64-darwin>     if (ptsname_r(
lnav-x86_64-darwin>         ^~~~~~~~~
lnav-x86_64-darwin>         ttyname_r
lnav-x86_64-darwin> /nix/store/6fb3yzhx0dlhzjk8s1mwkmn505yqbmmb-apple-sdk-10.12.2/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/unistd.h:488:6: note: 'ttyname_r' declared here
lnav-x86_64-darwin> int      ttyname_r(int, char *, size_t) __DARWIN_ALIAS(ttyname_r);
lnav-x86_64-darwin>          ^
lnav-x86_64-darwin> 1 error generated.
lnav-x86_64-darwin> make[3]: *** [Makefile:1675: readline_curses.o] Error 1
lnav-x86_64-darwin> make[3]: *** Waiting for unfinished jobs....
lnav-x86_64-darwin> make[3]: Leaving directory '/private/tmp/nix-build-lnav-0.12.3.drv-1/source/src'
lnav-x86_64-darwin> make[2]: *** [Makefile:1708: all-recursive] Error 1
lnav-x86_64-darwin> make[2]: Leaving directory '/private/tmp/nix-build-lnav-0.12.3.drv-1/source/src'
lnav-x86_64-darwin> make[1]: *** [Makefile:1344: all] Error 2
lnav-x86_64-darwin> make[1]: Leaving directory '/private/tmp/nix-build-lnav-0.12.3.drv-1/source/src'
lnav-x86_64-darwin> make: *** [Makefile:522: all-recursive] Error 1

This comes from the 10.12 macOS SDK, so the darwinMinVersionHook won't help here (that sets the minimum deployment target, not the SDK), you'll need this:

diff --git a/pkgs/by-name/ln/lnav/package.nix b/pkgs/by-name/ln/lnav/package.nix
index 628703a6e50a..1cbe887b1f07 100644
--- a/pkgs/by-name/ln/lnav/package.nix
+++ b/pkgs/by-name/ln/lnav/package.nix
@@ -19,7 +19,7 @@
   cargo,
   rustPlatform,
   rustc,
-  darwinMinVersionHook,
+  apple-sdk_11,
 }:
 
 stdenv.mkDerivation rec {
@@ -59,7 +59,7 @@ stdenv.mkDerivation rec {
       libarchive
     ]
     ++ lib.optionals stdenv.isDarwin [
-      (darwinMinVersionHook "10.13")
+      apple-sdk_11
     ]
     ++ lib.optionals (!stdenv.isDarwin) [
       gpm

@pcasaretto
Copy link
Contributor Author

Thanks @ofalvai!
I did run into another problem trying to cross-compile locally but I'm not confident in my ability to do that.
Used nix-build -A pkgsCross.x86_64-darwin.lnav
Got a ld: symbol(s) not found for architecture arm64 for the rustc dep.
https://gist.github.com/pcasaretto/b8dfb811586ca03999964734cb300f3e

@github-actions github-actions bot removed 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Dec 10, 2024
@ofalvai
Copy link
Contributor

ofalvai commented Dec 10, 2024

I'm not sure cross-compilation fully works between two Darwin archs, but you can use Rosetta for testing with the --system x86_64-darwin flag

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Diff LGTM.

@Atemu
Copy link
Member

Atemu commented Dec 10, 2024

You can also use pkgsx86_64Darwin btw.

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Dec 10, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Dec 11, 2024
@wegank wegank added the 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. label Dec 11, 2024
@Aleksanaa Aleksanaa merged commit 9ddc9fe into NixOS:master Dec 12, 2024
4 of 5 checks passed
@pcasaretto pcasaretto deleted the lnav-prql branch December 17, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants