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

lnav: add a few missing dependencies #357675

Merged
merged 4 commits into from
Nov 23, 2024
Merged

lnav: add a few missing dependencies #357675

merged 4 commits into from
Nov 23, 2024

Conversation

symphorien
Copy link
Member

also add a few missing optional dependencies
enable separateDebugInfo as lnav crashes not unfrequently.
add myself as maintainer

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@symphorien symphorien changed the title lnav: 0.12.2 -> 0.12.3 lnav: add a few missing dependencies Nov 22, 2024
@symphorien symphorien requested a review from Atemu November 22, 2024 21:44
@Atemu
Copy link
Member

Atemu commented Nov 23, 2024

I don't know the program enough to know what these deps are useful for but it looks reasonable to me.

Could you split the logical steps you've taken into separate commits?

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 23, 2024
@symphorien
Copy link
Member Author

I split the commits.

About the new dependencies here is the diff of the log outpot of ./configure:

--- logold	2024-11-23 17:33:36.605402229 +0100
+++ log	2024-11-23 17:33:33.509357416 +0100
@@ -31,7 +31,7 @@
 autoreconf: Leaving directory '.'
 patching script interpreter paths in ./configure
 ./configure: interpreter directive changed from "#! /bin/sh" to "/nix/store/0irlcqx2n3qm6b1pc9rsd2i8qpvcccaj-bash-5.2p37/bin/sh"
-configure flags: --disable-static --disable-dependency-tracking --prefix=/nix/store/i89q3v4ap60ikljgsdr8xw3ky66qywjg-lnav-0.12.3
+configure flags: --disable-static --disable-dependency-tracking --prefix=/nix/store/2qb6126273qk4g2xmsck7c5a656jcqyy-lnav-0.12.3 --bindir=/nix/store/2qb6126273qk4g2xmsck7c5a656jcqyy-lnav-0.12.3/bin --sbindir=/nix/store/2qb6126273qk4g2xmsck7c5a656jcqyy-lnav-0.12.3/sbin --includedir=/nix/store/2qb6126273qk4g2xmsck7c5a656jcqyy-lnav-0.12.3/include --oldincludedir=/nix/store/2qb6126273qk4g2xmsck7c5a656jcqyy-lnav-0.12.3/include --mandir=/nix/store/2qb6126273qk4g2xmsck7c5a656jcqyy-lnav-0.12.3/share/man --infodir=/nix/store/2qb6126273qk4g2xmsck7c5a656jcqyy-lnav-0.12.3/share/info --docdir=/nix/store/2qb6126273qk4g2xmsck7c5a656jcqyy-lnav-0.12.3/share/doc/lnav --libdir=/nix/store/2qb6126273qk4g2xmsck7c5a656jcqyy-lnav-0.12.3/lib --libexecdir=/nix/store/2qb6126273qk4g2xmsck7c5a656jcqyy-lnav-0.12.3/libexec --localedir=/nix/store/2qb6126273qk4g2xmsck7c5a656jcqyy-lnav-0.12.3/share/locale
 checking for a BSD-compatible install... /nix/store/k48bha2fjqzarg52picsdfwlqx75aqbb-coreutils-9.5/bin/install -c
 checking whether build environment is sane... yes
 checking for a race-free mkdir -p... /nix/store/k48bha2fjqzarg52picsdfwlqx75aqbb-coreutils-9.5/bin/mkdir -p
@@ -85,7 +85,7 @@
 checking for bash... /nix/store/0irlcqx2n3qm6b1pc9rsd2i8qpvcccaj-bash-5.2p37/bin/bash
 checking for cargo... no
 checking for bzip2... /nix/store/y59bxw99pbld247dznzv352cqdhlybqi-bzip2-1.0.8-bin/bin/bzip2
-checking for re2c... no
+checking for re2c... /nix/store/m86phdxnx92wr71cj08xybyi4axm0pdi-re2c-3.1/bin/re2c
 checking for xz... /nix/store/g6rf9zyrz8hrgz4r7sd2hab3j1afxdkr-xz-5.6.3-bin/bin/xz
 checking for tshark... no
 checking for check-jsonschema... no
@@ -118,8 +118,7 @@
 configure: Trying to link with tinfo
 checking for library containing cur_term... -ltinfo
 configure: Linking with tinfo
-checking for library containing Gpm_Open... no
-configure: WARNING: libgpm not found. If build fails later consider installing gpm dev package
+checking for library containing Gpm_Open... -lgpm
 checking for execinfo.h... yes
 checking for pty.h... yes
 checking for util.h... no
@@ -133,11 +132,9 @@
 checking for working ncursesw.h... no
 checking for working ncurses.h... yes
 checking lib archive... (testing)
-checking for archive_read_new in -larchive... no
-checking for archive.h... no
-checking for archive_read_new in -larchive... (cached) no
-checking for archive.h... no
-checking lib archive... no
+checking for archive_read_new in -larchive... yes
+checking for archive.h... yes
+checking lib archive... -larchive
 checking if PCRE2 is wanted... yes
 checking for pcre2_compile_8 in -lpcre2-8... yes
 checking for pcre2.h... yes
@@ -153,8 +150,6 @@
 checking for sqlite3_value_subtype... yes
 checking for sqlite3_error_offset... yes
 checking for sqlite3_drop_modules... yes
-configure: Checking for libgpm dependency
-checking for mousemask in -lncursesw... yes
 checking that generated files are newer than configure... done
 configure: creating ./config.status
 config.status: creating Makefile

they are probed by ./configure
lnav crashes more than the average program, let's say
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.

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 357675


x86_64-linux

✅ 2 packages built:
  • lnav
  • lnav.debug

@Atemu Atemu merged commit af6f9b7 into NixOS:master Nov 23, 2024
16 of 17 checks passed
@symphorien symphorien deleted the lnav branch November 23, 2024 18:09
@pcasaretto
Copy link
Contributor

It seems gpm is not available on darwin, so it broke the build.
Why didn't the build system build this for all archs?

@Atemu
Copy link
Member

Atemu commented Nov 27, 2024

In addition to ofBorg eval being slow as heck, its Darwin builders are currently also too slow to rely on.

Even if it worked, it's not immediately obvious whether a build failing is a regression or not; we have many many packages that don't eval or build on Darwin.

Please also note that Darwin is a tier 2 platform (NixOS/rfcs#46), so support is more of a best-effort sort of thing. It's not unexpected that it breaks every now and then.

If you'd like, you can add yourself to the package's maintainers in order to be pinged to review future PRs. Most people who contribute to Nixpkgs don't have a mac, so it's good to have some variety in that regard.

The fix for this should be trivial: Make the dep optional and disabled when stdenv.isDarwin.

@sagikazarmark
Copy link
Member

I opened #360350, it fixes the problem for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants