-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
texinfo: 7.0.3 -> 7.1 #262276
Conversation
pkgs/top-level/all-packages.nix
Outdated
texinfo7_0 = callPackage ../development/tools/misc/texinfo/7.0.nix { }; | ||
texinfo7 = callPackage ../development/tools/misc/texinfo/7.1.nix { }; |
There was a problem hiding this comment.
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:
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
pkgs/top-level/all-packages.nix
Outdated
@@ -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 { }; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Not sure if it's just
Does it work for you by chance? |
Thanks for testing, @trofi. I'm seeing the same issue as you are when trying to run It seems the subroutine in question (among others) was prefixed with Applying the patch below in
|
ℹ️ Testing the proposed changes here with ffmpeg 6.1 |
Sure! I applied both to
Does it build for you? |
Please remove the merge commit from the PR. It could cause issues if a merge conflict is on it. |
Of course. Thanks for calling this out, @SuperSandro2000, the merge commit has now been removed. |
Friendly ping to @AndersonTorres and @SuperSandro2000 on this 🙂 |
Let's send it to the Discourse thread. |
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 |
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 |
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 |
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 |
@infinisil sorry to bother you, can you recommend what to do about by-name check in this case ? |
Would moving texinfo into the |
No, not good. Use this instead: |
Very interesting, @AndersonTorres, thanks for sharing. Will this also address/fix the by-name check failures? |
Also could you provide more context, please, on why moving texinfo into the by-name tree is bad? I'm genuinely curious. |
@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? |
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. |
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 Co-authored-by: Anderson Torres <torres.anderson.85@protonmail.com>
Closing as #289690 got merged. |
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.
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)