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

texinfo: 7.0.3 -> 7.1 #262276

Closed
wants to merge 1 commit into from
Closed

texinfo: 7.0.3 -> 7.1 #262276

wants to merge 1 commit into from

Conversation

afh
Copy link
Member

@afh afh commented Oct 20, 2023

Description of changes

Diff: http://git.savannah.gnu.org/cgit/texinfo.git/diff/?id=texinfo-7.0.3&id2=texinfo-7.1

Changelog: http://git.savannah.gnu.org/cgit/texinfo.git/tree/ChangeLog?h=texinfo-7.1

Things done

Add texinfo 7.1 and make it the default.

  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

Comment on lines 20292 to 20293
texinfo7_0 = callPackage ../development/tools/misc/texinfo/7.0.nix { };
texinfo7 = callPackage ../development/tools/misc/texinfo/7.1.nix { };
Copy link
Member

Choose a reason for hiding this comment

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

Let's be more explicit:

Suggested change
texinfo7_0 = callPackage ../development/tools/misc/texinfo/7.0.nix { };
texinfo7 = callPackage ../development/tools/misc/texinfo/7.1.nix { };
texinfo7_0 = callPackage ../development/tools/misc/texinfo/7.0.nix { };
texinfo7_1 = callPackage ../development/tools/misc/texinfo/7.1.nix { };
texinfo = texinfo7_1;

Copy link
Member Author

Choose a reason for hiding this comment

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

@AndersonTorres would like to commit your suggestion, yet the message I receive over and over again when trying to do so is: This diff has recently been updated. Refresh and try again.

Are you okay with me making the change locally and including you suggestion in a single commit?

Copy link
Member

@AndersonTorres AndersonTorres Oct 20, 2023

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Are you okay with me making the change locally and including you suggestion in a single commit?

It should have been squashed anyway

@afh afh requested a review from AndersonTorres October 20, 2023 20:06
@@ -20289,8 +20289,9 @@ with pkgs;
texinfo6_5 = callPackage ../development/tools/misc/texinfo/6.5.nix { }; # needed for allegro
texinfo6_7 = callPackage ../development/tools/misc/texinfo/6.7.nix { }; # needed for gpm, iksemel and fwknop
texinfo6 = callPackage ../development/tools/misc/texinfo/6.8.nix { };
texinfo7 = callPackage ../development/tools/misc/texinfo/7.0.nix { };
texinfo = texinfo7;
texinfo7_0 = callPackage ../development/tools/misc/texinfo/7.0.nix { };
Copy link
Member

Choose a reason for hiding this comment

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

Let me ask a naive question: do we even need 7.0 right now?

Copy link
Member Author

Choose a reason for hiding this comment

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

After giving it some thought, @SuperSandro2000, I believe that texinfo7_0 could be useful to have, so it can be used by ffmpeg-headless, until ffmpeg docs are patched upstream to work with texinfo 7.1 (see my comment below).

Copy link
Member

Choose a reason for hiding this comment

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

Then I would suggest to use it right away and add a comment when we can switch to 7.1

Copy link
Member Author

Choose a reason for hiding this comment

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

@SuperSandro2000 What are your thoughts around removing texinfo7_0 in favor of texinfo7_1 and using the patch below?

Copy link
Member

Choose a reason for hiding this comment

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

Do not remove 7.0 if it is still needed.

Or at least remove it in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

We can also do this later.

@ofborg ofborg bot requested a review from SuperSandro2000 October 21, 2023 02:21
@trofi
Copy link
Contributor

trofi commented Oct 23, 2023

Not sure if it's just texonfo-7.1 update or some extra patches I have locally, but I am seeing the following failure against this PR on ffmpeg-headless:

ffmpeg-headless> makeinfo: error parsing ./doc/t2h.pm: Undefined subroutine &Texinfo::Config::set_from_init_file called at ./doc/t2h.pm line 24.
ffmpeg-headless> POD    doc/libavfilter.pod
ffmpeg-headless> make: *** [doc/Makefile:69: doc/libavformat.html] Error 1 shuffle=3953956671
ffmpeg-headless> make: *** Waiting for unfinished jobs....

Does it work for you by chance?

@afh
Copy link
Member Author

afh commented Oct 29, 2023

Thanks for testing, @trofi. I'm seeing the same issue as you are when trying to run ffmpeg-headless in combination with the changes in this PR.

It seems the subroutine in question (among others) was prefixed with texinfo_ (see this commit) in September 2021 along with other refactorings that break producing the FFmpeg docs.

Applying the patch below in pkgs/development/libraries/ffmpeg/generic.nix fixes the issue. What is the best course of action from your point of view?
Report the patch to FFmpeg upstream and add it to the ffmpeg nix package for the time being?

fix-t2h-for-texinfo-7_1.patch
diff --git a/doc/t2h.pm b/doc/t2h.pm
index d07d974..fc85e04 100644
--- a/doc/t2h.pm
+++ b/doc/t2h.pm
@@ -21,7 +21,7 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
 # no navigation elements
-set_from_init_file('HEADERS', 0);
+texinfo_set_from_init_file('HEADERS', 0);
 
 sub ffmpeg_heading_command($$$$$)
 {
@@ -55,8 +55,9 @@ sub ffmpeg_heading_command($$$$$)
         $element = $command->{'parent'};
     }
     if ($element) {
-        $result .= &{$self->{'format_element_header'}}($self, $cmdname,
-                                                       $command, $element);
+        $result .= &{$self->formatting_function('format_element_header')}($self,
+                                                       $cmdname, $command,
+                                                       $element);
     }
 
     my $heading_level;
@@ -112,8 +113,8 @@ sub ffmpeg_heading_command($$$$$)
                 $cmdname
                     = $Texinfo::Common::level_to_structuring_command{$cmdname}->[$heading_level];
             }
-            $result .= &{$self->{'format_heading_text'}}(
-                        $self, $cmdname, $heading,
+            $result .= &{$self->formatting_function('format_heading_text')}(
+                        $self, $cmdname, undef, $heading,
                         $heading_level +
                         $self->get_conf('CHAPTER_HEADER_LEVEL') - 1, $command);
         }
@@ -127,22 +128,22 @@ foreach my $command (keys(%Texinfo::Common::sectioning_commands), 'node') {
 }
 
 # determine if texinfo is at least version 6.8
-my $program_version_num = version->declare(get_conf('PACKAGE_VERSION'))->numify;
+my $program_version_num = version->declare(texinfo_get_conf('PACKAGE_VERSION'))->numify;
 my $program_version_6_8 = $program_version_num >= 6.008000;
 
 # print the TOC where @contents is used
 if ($program_version_6_8) {
-    set_from_init_file('CONTENTS_OUTPUT_LOCATION', 'inline');
+    texinfo_set_from_init_file('CONTENTS_OUTPUT_LOCATION', 'inline');
 } else {
-    set_from_init_file('INLINE_CONTENTS', 1);
+    texinfo_set_from_init_file('INLINE_CONTENTS', 1);
 }
 
 # make chapters <h2>
-set_from_init_file('CHAPTER_HEADER_LEVEL', 2);
+texinfo_set_from_init_file('CHAPTER_HEADER_LEVEL', 2);
 
 # Do not add <hr>
-set_from_init_file('DEFAULT_RULE', '');
-set_from_init_file('BIG_RULE', '');
+texinfo_set_from_init_file('DEFAULT_RULE', '');
+texinfo_set_from_init_file('BIG_RULE', '');
 
 # Customized file beginning
 sub ffmpeg_begin_file($$$)
@@ -159,7 +160,7 @@ sub ffmpeg_begin_file($$$)
     my ($title, $description, $encoding, $date, $css_lines,
         $doctype, $bodytext, $copying_comment, $after_body_open,
         $extra_head, $program_and_version, $program_homepage,
-        $program, $generator) = $self->_file_header_informations($command);
+        $program, $generator) = $self->_file_header_information($command);
 
     my $links = $self->_get_links ($filename, $element);
 
@@ -223,7 +224,7 @@ if ($program_version_6_8) {
 sub ffmpeg_end_file($)
 {
     my $self = shift;
-    my $program_string = &{$self->{'format_program_string'}}($self);
+    my $program_string = &{$self->formatting_function('format_program_string')}($self);
     my $program_text = <<EOT;
       <p style="font-size: small;">
         $program_string
@@ -244,7 +245,7 @@ if ($program_version_6_8) {
 
 # Dummy title command
 # Ignore title. Title is handled through ffmpeg_begin_file().
-set_from_init_file('USE_TITLEPAGE_FOR_TITLE', 1);
+texinfo_set_from_init_file('USE_TITLEPAGE_FOR_TITLE', 1);
 sub ffmpeg_title($$$$)
 {
     return '';
@@ -262,7 +263,7 @@ sub ffmpeg_float($$$$$)
     my $args = shift;
     my $content = shift;
 
-    my ($caption, $prepended) = Texinfo::Common::float_name_caption($self,
+    my ($caption, $prepended) = Texinfo::Convert::Converter::float_name_caption($self,
                                                                 $command);
     my $caption_text = '';
     my $prepended_text;
@@ -335,21 +336,21 @@ sub ffmpeg_float($$$$$)
             $caption->{'args'}->[0], 'float caption');
     }
     if ($prepended_text.$caption_text ne '') {
-        $prepended_text = $self->_attribute_class('div','float-caption'). '>'
+        $prepended_text = $self->html_attribute_class('div', ['float-caption']). '>'
                 . $prepended_text;
         $caption_text .= '</div>';
     }
-    my $html_class = '';
+    my @html_class = ();
     if ($prepended_save =~ /NOTE/) {
-        $html_class = 'info';
+        push @html_class, 'info';
         $prepended_text = '';
         $caption_text   = '';
     } elsif ($prepended_save =~ /IMPORTANT/) {
-        $html_class = 'warning';
+        push @html_class, 'warning';
         $prepended_text = '';
         $caption_text   = '';
     }
-    return $self->_attribute_class('div', $html_class). '>' . "\n" .
+    return $self->html_attribute_class('div', \@html_class). '>' . "\n" .
         $prepended_text . $caption_text . $content . '</div>';
 }

@afh
Copy link
Member Author

afh commented Nov 30, 2023

ℹ️ Testing the proposed changes here with ffmpeg 6.1

@afh
Copy link
Member Author

afh commented Dec 1, 2023

I was able to successfully build and run ffmpeg-headless version 6.1 (see #271399) with the changes proposed here.

@trofi would you be willing to test the two PRs (this and #271399) in combination?

@trofi
Copy link
Contributor

trofi commented Dec 1, 2023

Sure! I applied both to staging and got ffmped_6 failure as:

$ nix build --no-link -f. ffmpeg_6 --keep-going

error: builder for '/nix/store/nl2h321i26ccgmdbfwi9br1qh0m4dgkk-ffmpeg-headless-6.1.drv' failed with exit code 1;
       last 10 log lines:
       > Running phase: configurePhase
       > patching script interpreter paths in ./configure
       > configure flags: --disable-static --prefix=/nix/store/8lf8dsd42v79p9z0mgwnifhyybag32jh-ffmpeg-headless-6.1 --target_os=linux --arch=x86_64 --pkg-config=pkg-config --enable-gpl --enable-version3 --disable-nonfree --enable-shared --enable-pic --disable-small --enable-runtime-cpudetect --disable-gray --enable-swscale-alpha --enable-hardcoded-tables --enable-safe-bitstream-reader --enable-pthreads --disable-w32threads --disable-os2threads --enable-network --enable-pixelutils --datadir=/nix/store/svy6ssg03rz40hz743xwznhsam4hidgg-ffmpeg-headless-6.1-data/share/ffmpeg --enable-ffmpeg --disable-ffplay --enable-ffprobe --bindir=/nix/store/7rhxw2llf53jwcpf7hqr78aal6kw101x-ffmpeg-headless-6.1-bin/bin --enable-avcodec --enable-avdevice --enable-avfilter --enable-avformat --enable-avutil --enable-postproc --enable-swresample --enable-swscale --libdir=/nix/store/c596idk22gd4wni7krvhkx6qfwzbxpsl-ffmpeg-headless-6.1-lib/lib --incdir=/nix/store/gaqkwwq9pmqaa6zljrbn2bxn07zf9sdl-ffmpeg-headless-6.1-dev/include --enable-doc --enable-htmlpages --enable-manpages --mandir=/nix/store/mi6x2ykpvb3yrr7jgc3725v31k74d763-ffmpeg-headless-6.1-man/share/man --enable-podpages --enable-txtpages --docdir=/nix/store/frg6p28dav1i2l7fbx42fn6758b2pcmn-ffmpeg-headless-6.1-doc/share/doc/ffmpeg --enable-alsa --enable-bzlib --disable-libcelt --disable-cuda --disable-cuda-llvm --enable-libdav1d --disable-libfdk-aac --disable-libflite --enable-fontconfig --enable-libfreetype --disable-frei0r --disable-libfribidi --disable-libgme --enable-gnutls --disable-libgsm --disable-ladspa --enable-libmp3lame --disable-libaom --enable-libass --disable-libbluray --disable-libbs2b --disable-libdc1394 --enable-libdrm --enable-iconv --disable-libjack --disable-libmfx --disable-libmodplug --disable-libmysofa --enable-libopus --disable-librsvg --enable-libsrt --enable-libssh --disable-libtensorflow --enable-libtheora --enable-libv4l2 --enable-v4l2-m2m --enable-vaapi --disable-vdpau --enable-libvorbis --disable-libvmaf --enable-libvpx --disable-libwebp --disable-xlib --disable-libxcb --disable-libxcb-shm --disable-libxcb-xfixes --disable-libxcb-shape --disable-libxml2 --enable-lzma --enable-cuvid --enable-nvdec --enable-nvenc --disable-openal --disable-opencl --disable-libopencore-amrnb --disable-opengl --disable-libopenh264 --disable-libopenjpeg --disable-libopenmpt --disable-libpulse --disable-librav1e --enable-libsvtav1 --disable-librtmp --disable-sdl2 --enable-libsoxr --enable-libspeex --disable-libvidstab --disable-libvo-amrwbenc --enable-libx264 --enable-libx265 --disable-libxavs --enable-libxvid --disable-libzmq --enable-libzimg --enable-zlib --disable-vulkan --disable-libglslang --disable-libsmbclient --disable-debug --enable-optimizations --disable-extra-warnings --disable-stripping
       > ERROR: cuvid requested, but not all dependencies are satisfied: ffnvcodec
       >
       > If you think configure made a mistake, make sure you are using the latest
       > version from Git.  If the latest version fails, report the problem to the
       > ffmpeg-user@ffmpeg.org mailing list or IRC #ffmpeg on irc.libera.chat.
       > Include the log file "ffbuild/config.log" produced by configure as this will help
       > solve the problem.
       For full logs, run 'nix log /nix/store/nl2h321i26ccgmdbfwi9br1qh0m4dgkk-ffmpeg-headless-6.1.drv'.

Does it build for you?

@ofborg ofborg bot requested a review from bjornfor December 1, 2023 18:27
@SuperSandro2000
Copy link
Member

Please remove the merge commit from the PR. It could cause issues if a merge conflict is on it.

@afh
Copy link
Member Author

afh commented Dec 3, 2023

Of course. Thanks for calling this out, @SuperSandro2000, the merge commit has now been removed.

@ofborg ofborg bot requested a review from AndersonTorres December 4, 2023 20:46
@afh
Copy link
Member Author

afh commented Dec 11, 2023

Friendly ping to @AndersonTorres and @SuperSandro2000 on this 🙂

@AndersonTorres
Copy link
Member

Let's send it to the Discourse thread.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3208

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1370

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1446

@kirillrdy
Copy link
Member

@afh please add link to Diff / change log to commit message and PR, eg 44bee06

@afh
Copy link
Member Author

afh commented Feb 17, 2024

Sure thing, @kirillrdy, the commit message and PR description have been updated.

Not that the diff link seems to work, but stalls for me after some time, not sure if the diff is too large…

@kirillrdy
Copy link
Member

Sure thing, @kirillrdy, the commit message and PR description have been updated.

Not that the diff link seems to work, but stalls for me after some time, not sure if the diff is too large…

for major releases link to chagelog/release log is sufficient. as you stated, if diff is too large, then its probably not that useful

@kirillrdy
Copy link
Member

@infinisil sorry to bother you, can you recommend what to do about by-name check in this case ?

@afh
Copy link
Member Author

afh commented Feb 17, 2024

Would moving texinfo into the pkgs/by-name tree be something people would find acceptable? I'd be happy to draft a PR, if folks see value in it.

@AndersonTorres
Copy link
Member

No, not good. Use this instead:

#287918

#287918 (comment)

@afh
Copy link
Member Author

afh commented Feb 17, 2024

Very interesting, @AndersonTorres, thanks for sharing. Will this also address/fix the by-name check failures?

@afh
Copy link
Member Author

afh commented Feb 17, 2024

Also could you provide more context, please, on why moving texinfo into the by-name tree is bad? I'm genuinely curious.

@afh
Copy link
Member Author

afh commented Feb 17, 2024

@AndersonTorres, I have some local changes for texinfo in the same vein as the PRs you referenced. Should this PR be re-used for that or would a new (draft?) PR be more appropriate?

@afh
Copy link
Member Author

afh commented Feb 18, 2024

Eager to learn more about nixpkgs and motivated by @AndersonTorres' comment I drafted #289690. Since you, @AndersonTorres and @kirillrdy, have been so kind to share your knowledge and expertise on this PR recently, maybe you'd like to leave a feedback on the draft PR #289690 as well; I'd appreciate it greatly.

@afh afh force-pushed the update-texinfo branch from cc3c3ce to d60f2c9 Compare June 5, 2024 12:29
@ofborg ofborg bot requested review from cpages, amiloradovsky and gebner June 5, 2024 14:52
@afh
Copy link
Member Author

afh commented Jun 26, 2024

Closing as #289690 got merged.

@afh afh closed this Jun 26, 2024
@afh afh deleted the update-texinfo branch June 26, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants