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.
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.
@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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Let me ask a naive question: do we even need 7.0 right now?
There was a problem hiding this comment.
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.
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.
@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.
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.
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 = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)