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

aegisub: 3.3.3 -> 3.4.0 #359027

Merged
merged 3 commits into from
Dec 19, 2024
Merged

aegisub: 3.3.3 -> 3.4.0 #359027

merged 3 commits into from
Dec 19, 2024

Conversation

emilazy
Copy link
Member

@emilazy emilazy commented Nov 25, 2024

Aegisub is a program of many forks. We currently use the inactive wangqr fork that has not received an update in two years. This switches to the actively‐maintained arch1t3cht fork, bringing in additional functionality, bug fixes, and a much more reasonable Meson build system that lets us drop our LuaJIT devendoring patch. It also contains fixes for newer versions of Boost.

Note that although the README.md has a note from two years ago saying “Don't use this version if you're just looking for any version of Aegisub”, that is somewhat outdated and predates it becoming a fully‐fledged fork: as the next section of the file from two months ago admits, it is the current de facto standard Aegisub fork and the version universally recommended by up‐to‐date sources; see e.g.:

cc @al3xtjames – I see you’ve worked on packaging this for Nix and contributed upstream to assist with that; perhaps you’d be interested in maintaining this package? :) (I expect your version may have benefits over this, from a quick skim; I just did this in the process of Boost clean‐up, but I’d be happy to review a PR bringing it in line with your changes.)

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added 8.has: clean-up 8.has: package (new) This PR adds a new package labels Nov 26, 2024
@ofborg ofborg bot requested review from sbruder, wegank, rnhmjoj and snaakey November 26, 2024 09:01
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Nov 26, 2024
@@ -167,5 +144,7 @@ stdenv.mkDerivation (finalAttrs: {
mainProgram = "aegisub";
maintainers = with lib.maintainers; [ wegank ];
platforms = lib.platforms.unix;
# No native wxWidgets and VapourSynth is broken.
Copy link
Member

Choose a reason for hiding this comment

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

I'm unable to test VapourSynth on darwin but if its broken maybe also add badPlatforms to the VapourSynth package.

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s already listed as broken there (not badPlatforms, though, which might be slightly better).

Copy link
Member

Choose a reason for hiding this comment

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

Oh nvm I should have looked at the whole file before commenting

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Nov 26, 2024
@emilazy
Copy link
Member Author

emilazy commented Nov 28, 2024

Sadly they compile code specific to the wxWidgets Cocoa backend on macOS so even after @wegank’s work to get VapourSynth working on Darwin this still won’t go. Would be nice to get that backend packaged at some point.

@wegank
Copy link
Member

wegank commented Nov 28, 2024

I think wangqr/Aegisub@3556c96 would help fix aegisub on darwin.

(Also, I think I've mentioned this before, but wxGTK32 is a misnomer on Darwin, as it doesn't depend on GTK at all. Renaming it to wxwidgets_3_2 would probably be a good idea...)

@emilazy
Copy link
Member Author

emilazy commented Nov 28, 2024

I think wangqr/Aegisub@3556c96 would help fix aegisub on darwin.

Aha, a regression, interesting! I guess I should port that to the active fork then :)

(Also, I think I've mentioned this before, but wxGTK32 is a misnomer on Darwin, as it doesn't depend on GTK at all. Renaming it to wxwidgets_3_2 would probably be a good idea...)

Oh, I had no idea and just assumed. That is indeed extremely confusing.

@emilazy
Copy link
Member Author

emilazy commented Nov 28, 2024

Looks like it was intentionally undone: arch1t3cht/Aegisub@6dda04d.

I wonder if there’s anything we can do to get at the private symbols here? (Or we could just re‐re‐re‐revert it ourselves and choose broken IME support over a broken build.)

@al3xtjames
Copy link
Contributor

al3xtjames commented Nov 28, 2024

IIRC that commit was reverted in the fork to fix IME input. Unfortunately it uses private symbols from wxWidgets which causes linking to fail when using a non-bundled version. I worked around this by using the bundled wx on Darwin, though this isn't ideal...

(Looks like we collided).

@snaakey snaakey mentioned this pull request Nov 28, 2024
13 tasks
@kini
Copy link
Member

kini commented Dec 19, 2024

Looks like upstream is no longer dead, and I guess is reabsorbing arch1t3cht's fork? https://aegisub.org/blog/aegisub-3.4.0-released

Seems to build fine on AArch64.
Aegisub is a program of many forks. We currently use the inactive
wangqr fork that has not received an update in two years. This
switches to the actively‐maintained arch1t3cht fork, bringing in
additional functionality, bug fixes, and a much more reasonable Meson
build system that lets us drop our LuaJIT devendoring patch. It also
contains fixes for newer versions of Boost.

Note that although the `README.md` has a note from two years ago saying
“**Don't** use this version if you're just looking for any version
of Aegisub”, that is somewhat outdated and predates it becoming a
fully‐fledged fork: as the next section of the file from two months
ago admits, it is the current de facto standard Aegisub fork and the
version universally recommended by up‐to‐date sources; see e.g.:

* <https://fansubbers.miraheze.org/wiki/Aegisub>
* <https://guide.encode.moe/typesetting/aegisub.html>
* <https://www.reddit.com/r/aegisub/comments/yzvi4m/aegisub_update_status/>
* <Ristellise/AegisubDC#10>
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1 labels Dec 19, 2024
@wegank
Copy link
Member

wegank commented Dec 19, 2024

@ofborg build aegisub aegisub.passthru.tests

@wegank
Copy link
Member

wegank commented Dec 19, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 359027


aarch64-darwin

✅ 3 packages built:
  • aegisub
  • vapoursynth-bestsource
  • vapoursynth-bestsource.dev

@wegank wegank changed the title aegisub: switch to arch1t3cht’s fork aegisub: 3.3.3 -> 3.4.0 Dec 19, 2024
@wegank wegank merged commit a56195a into NixOS:master Dec 19, 2024
34 of 36 checks passed
@emilazy
Copy link
Member Author

emilazy commented Dec 27, 2024

Hmm, I feel a little strange about what happened to this PR while I was gone.

The new upstream is the TypesettingTools fork, which I already considered while making this PR. It’s definitely what we should be using in the long run, and I’m glad that the Aegisub ecosystem is finally converging on one repository. However, as the blog post itself says, it does not include all the changes from arch1t3cht’s fork yet:

The goal was to just cut a release that builds on modern systems, which by itself was more than enough work. Further releases will include actual features and more normal changelogs.

arch1t3cht has done a great job of running the primary fork used over the past few years, and now has commit access in TypesettingTools so that he can work out of a more centralized repository. We’d love to get all the work done across various forks back into the mainline repository, so please send PRs!

The arch1t3cht fork is still hundreds of commits ahead of the TypesettingTools repository, and the only changes to the latter since I opened this PR were some bug fix commits. I think the correct thing to do here is to continue to use arch1t3cht’s fork for now, for the reasons I gave in the original PR message, and then switch to the TypesettingTools repository in a release or two once it has absorbed a meaningful amount of the functionality in the fork. Mostly though it just feels a little odd for my PR with the explicit intent of switching to the arch1t3cht fork for now to get reused for this – I wouldn’t mind if this one had been closed in favour of a PR taking the alternate approach.

@kini
Copy link
Member

kini commented Dec 27, 2024

I completely agree. I hope I'm wrong, but if by any chance my comment above was what prompted the repurposing of this PR from "aegisub: switch to arch1t3cht's fork" to "3.3.3 -> 3.4.0" and its subsequent merge, then I apologize as that was not my intention ― I just wanted to bring attention to the blog post announcing the release of 3.4.0 since I thought it relevant to the discussion.

I don't know if my $0.02 is worth anything since I'm basically just a driveby commenter on this PR 😅 but IMO the author of a PR should be the one to make any major changes to the goal or aim of the PR, and if someone wants to do something different it's easy enough to open a different PR for that.

@wegank
Copy link
Member

wegank commented Dec 29, 2024

I apologize for the quick and thoughtless bump to 3.4.0. Feel free to revert the latest commit or introduce the fork as a separate package.

@emilazy emilazy deleted the push-yzxtlkmknpvt branch January 3, 2025 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants