Skip to content

Comments

lnav: 0.11.2 -> 0.12.1#304752

Closed
cor wants to merge 1 commit intoNixOS:masterfrom
cor:lnav-v0.12.1
Closed

lnav: 0.11.2 -> 0.12.1#304752
cor wants to merge 1 commit intoNixOS:masterfrom
cor:lnav-v0.12.1

Conversation

@cor
Copy link
Contributor

@cor cor commented Apr 17, 2024

Description of changes

Changelog: https://github.com/tstack/lnav/releases/tag/v0.12.1

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@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. labels Apr 17, 2024
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Apr 17, 2024
Copy link
Member

@sikmir sikmir left a comment

Choose a reason for hiding this comment

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

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

1 package failed to build:
  • lnav
lnav> readline_curses.cc:839:9: error: use of undeclared identifier 'ptsname_r'; did you mean 'ttyname_r'?
lnav>     if (ptsname_r(
lnav>         ^~~~~~~~~
lnav>         ttyname_r

sikmir referenced this pull request in tstack/lnav Apr 20, 2024
@tstack
Copy link

tstack commented Apr 20, 2024

Can you add a dependency on rust/cargo. The 0.12.1 version adds support for PRQL, which is implemented in rust.

It seems like the darwin version is pretty recent, so I don't know why ptsname_r is not found. I think _GNU_SOURCE might be needed for it to be available and that is done in the configure script:

https://github.com/tstack/lnav/blob/4c3c958ab136136c5a1b626410486bc9dd1c29be/configure.ac#L33

Is it possible to run the make with V=1, so that the build commands are written out fully instead of the abbreviated CXX <filename>.

@tstack
Copy link

tstack commented Apr 20, 2024

It looks like a dependency on libarchive is also missing. That allows lnav to open up archive files and read the files within.

@tstack
Copy link

tstack commented May 2, 2024

Another issue is that a dependency on tzdata needs to be added and a patch done to make the date library in lnav look in the right place. @soenkeliebau has done that part here:

https://github.com/soenkeliebau/nixpkgs/blob/lnav_0.12.2/pkgs/tools/misc/lnav/default.nix

@soenkeliebau
Copy link

soenkeliebau commented May 2, 2024

Full disclosure: that patch should be taken with a fair amount of salt!
That was just the first simple thing that I could come up with to make this work. Not sure if for example there is a chance to hit that other codepath with a hardcoded default value.

Also, embedding the patch in the .nix file is kinda ugly, but can't be helped because otherwise ${tzdata} wouldn't be interpolated..

It would be nicer to come up with a generic patch that checks the TZDIR (which is set by tzdata automatically) env var and uses that instead of hardcoding a default.

samuel-emrys/date@9755167 has some code to that effect I believe. But my C skills are quite frankly not up to fully understanding what is happening and adapting it.

@tstack
Copy link

tstack commented May 3, 2024

Full disclosure: that patch should be taken with a fair amount of salt!

You say that, but it's not too different from what was done in the official nix package for the date library:

https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/libraries/howard-hinnant-date/default.nix

I ended up doing this:

https://github.com/tstack/lnav/blob/40a93ca3d398df80601b3382cd2776cd9f53049a/src/third-party/date/src/tz.cpp#L350-L353

Which is what was done here:

https://github.com/HowardHinnant/date/pull/579/files

@drupol drupol requested a review from sikmir May 12, 2024 19:33
@wegank
Copy link
Member

wegank commented Jul 3, 2024

Done in #323321.

@wegank wegank closed this Jul 3, 2024
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. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants