Skip to content

Conversation

@afh
Copy link
Member

@afh afh commented Feb 10, 2023

Change default texinfo to 7

Description of changes

Add package for texinfo 7.0.2 and make it the default.

Please chime in if you believe changing the default texinfo to 7.0.2 is too drastic of a change or should be communicated elsewhere than just the release notes.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation labels Feb 10, 2023
@afh
Copy link
Member Author

afh commented Feb 10, 2023

Here is a new PR as requested by @winterqt (for context see #215654). Do you have any idea of what may have gone wrong with a review having been requested from so many folks?

@ncfavier
Copy link
Member

I'm guessing you did not follow the instructions for rebasing smoothly.

As for the patch thing, the build log shows that some script is failing due to /usr/bin/env not being present in the sandbox. patchShebangs replaces /usr/bin/env shebangs with a direct call to the requested interpreter (in this case Perl).

@afh
Copy link
Member Author

afh commented Feb 10, 2023

Thanks for the explanation @ncfavier. I was not aware about the special procedure that is necessary when rebasing. I apologies for the unnecessary pings to people and disruption of their time and attention this has likely created. Mea culpa 😞

@ncfavier ncfavier requested a review from trofi February 10, 2023 19:21
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 8.has: package (new) This PR adds a new package labels Feb 10, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Feb 10, 2023
Copy link
Contributor

@trofi trofi left a comment

Choose a reason for hiding this comment

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

LGTM with a minor nit.

My naive grep says texinfo might be directly used by a few packages. I'll try a local build on them and complain back if something fails.

List I found: R allegro5 asdf asdf_2_26 asdf_3_1 asymptote autoconf autoconf271 bashInteractive bashInteractiveFHS bashInteractive_5 bc binutils-unwrapped_2_38 blitz cgdb cgui chrony cmakeCurses cmakeWithGui cre2 docbook2x e2fsprogs ecl emacs emacs-gtk emacs-nox emacs28 emacs28-gtk emacs28-nox emacs28NativeComp emacsNativeComp ffmpeg ffmpeg-full ffmpeg-headless ffmpeg_4 ffmpeg_4-full ffmpeg_4-headless ffmpeg_5 ffmpeg_5-full ffmpeg_5-headless flex_2_5_35 foxtrotgps freetalk frr fswatch fuse2fs gama gauche gaucheBootstrap gcl gcl_2_6_13_pre gdb gengetopt gforth git git-doc gitFull gitSVN glibcInfo gnu-cobol gnupg gnupg24 gnuplot gnuplot_aquaterm gnuplot_qt gpgme groff guile-config guile-fibers guile-gcrypt guile-git guile-hall guile-json guile-lib guile-sqlite3 guile-ssh guile-xcb gxmessage heimdal idutils indent irods irods-icommands jellyfin-ffmpeg jffi ledger lepton-eda libheimdal liblouis libmikmod libredwg librep libtasn1 lilypond lilypond-unstable linuxdoc-tools lzlib macchanger mailutils marst maxima maxima-ecl mercury mitscheme mitschemeX11 monotone mpfi nano ne netmask notmuch ocrad octave octaveFull pinentry-rofi pinfo plzip poke ponysay proxysql pspp qemu qemu_full qemu_kvm qemu_test quickjs recode rocgdb rubber sawfish sbcl sbcl_2_0_8 sbcl_2_0_9 sbcl_2_1_1 sbcl_2_1_10 sbcl_2_1_11 sbcl_2_1_2 sbcl_2_1_9 sbcl_2_2_10 sbcl_2_2_11 sbcl_2_2_4 sbcl_2_2_6 sbcl_2_2_9 sbcl_2_3_0 sdcc sgx-sdk solfege speechd tahoe-lafs taler-exchange taler-merchant tangogps tarlz tinc_pre tinycc udunits wdiff wget2 wipefreespace wsjtx xboard xnee xprintidle-ng xzgv zsh

Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

Fix the message of the commit: postPath -> postPatch

Copy link
Contributor

@trofi trofi left a comment

Choose a reason for hiding this comment

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

At least texinfoInteractive fails to build as:

XPASS: test_scripts/layout_formatting_fr_icons.sh
PASS: test_scripts/layout_formatting_epub.sh
PASS: test_scripts/layout_formatting.sh
PASS: test_scripts/layout_formatting_epub_nodes.sh
============================================================================
Testsuite summary for GNU Texinfo 7.0.2
============================================================================
# TOTAL: 144
# PASS:  112
# SKIP:  31
# XFAIL: 0
# FAIL:  0
# XPASS: 1
# ERROR: 0
============================================================================
See tp/tests/test-suite.log
Please report to [email protected]
============================================================================

AFAIU XPASS failure is unexpected.

@afh
Copy link
Member Author

afh commented Feb 11, 2023

@trofi I was able to build texinfoInteractive using nix shell nixpkgs-dev#texinfoInteractive. Which command(s) did you use to build texinfoInteractive that failed as you described?

@trofi
Copy link
Contributor

trofi commented Feb 11, 2023

@trofi I was able to build texinfoInteractive using nix shell nixpkgs-dev#texinfoInteractive. Which command(s) did you use to build texinfoInteractive that failed as you described?

it's an x86_64-linux system. Otherwise it should be equivalent to your's. I did it as:

$ nix build github:NixOS/nixpkgs/pull/215699/merge#texinfoInteractive
...
error: builder for '/nix/store/g8cvcfblamsd2b9cizrlmiv9q0ajxc2c-texinfo-interactive-7.0.2.drv' failed with exit code 2;

Maybe it's a locale-dependent failure.

@ncfavier
Copy link
Member

We just need to condition

checkFlags = lib.optionals (!stdenv.hostPlatform.isMusl) [
# Test is known to fail on various locales on texinfo-6.8:
# https://lists.gnu.org/r/bug-texinfo/2021-07/msg00012.html
"XFAIL_TESTS=test_scripts/layout_formatting_fr_icons.sh"
];
on version < 7.

@afh afh force-pushed the texinfo7 branch 2 times, most recently from 5c883a5 to 2549060 Compare February 11, 2023 12:35
@afh
Copy link
Member Author

afh commented Feb 11, 2023

@trofi mind retrying the texinfoInteractive build now that the suggestion from @ncfavier has been pushed to this PR?

@trofi
Copy link
Contributor

trofi commented Feb 11, 2023

@trofi mind retrying the texinfoInteractive build now that the suggestion from @ncfavier has been pushed to this PR?

Yup, that did it. Thank you!

Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

Except maybe by release note, LGTM.

@github-actions github-actions bot removed 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation labels Feb 12, 2023
@afh
Copy link
Member Author

afh commented Feb 12, 2023

Thanks everyone for your review, helpful comments, and suggestions, very much appreciated, especially as I learned a few new things about nixpkgs.

From my end this PR is good to be merged, if anyone with the necessary permissions agrees please do so at your latest convenience 🙂

@AndersonTorres
Copy link
Member

@ofborg build

@AndersonTorres
Copy link
Member

Should I we ignore Darwin failure?

@winterqt
Copy link
Member

@AndersonTorres @ofborg build doesn't do anything without arguments. You need @ofborg eval, which you did in a deleted comment.

@winterqt
Copy link
Member

Should I we ignore Darwin failure?

error: building of '/nix/store/64gbskxarzv0nm4zbv7ijhhfiqkvb47j-llvm-11.1.0.drv!lib,out' from .drv file timed out after 3600 seconds

Timed out as expected, but I can test on master.

@AndersonTorres
Copy link
Member

@AndersonTorres @ofborg build doesn't do anything without arguments. You need @ofborg eval, which you did in a deleted comment.

I believed it was just the opposite, eval needs args but build doesn't...

@afh
Copy link
Member Author

afh commented Feb 16, 2023

Just checking in on the status here… Anything that is needed from me or needs to be resolved before this can get merged?

@trofi
Copy link
Contributor

trofi commented Feb 16, 2023

I did not fine any regression so far. Should be fine to merge. Worst case we'll pin back the previous version of texinfo for those packages that end up failing to build.

@trofi trofi merged commit cb2b526 into NixOS:staging Feb 16, 2023
@afh afh deleted the texinfo7 branch February 17, 2023 06:07
@afh
Copy link
Member Author

afh commented Feb 17, 2023

Thanks everyone for all the helpful input to get this merged, very much appreciated 🙌
Happy to help out and do some legwork if something related to texinfo7 now being the default breaks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (new) This PR adds a new package 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants