Skip to content

fcitx5-mozc: 2.26.4220.102 -> 2.30.5544.102#346680

Merged
h7x4 merged 10 commits into
NixOS:masterfrom
musjj:fcitx5-mozc-new
Oct 27, 2024
Merged

fcitx5-mozc: 2.26.4220.102 -> 2.30.5544.102#346680
h7x4 merged 10 commits into
NixOS:masterfrom
musjj:fcitx5-mozc-new

Conversation

@musjj
Copy link
Copy Markdown
Contributor

@musjj musjj commented Oct 5, 2024

Upgrades fcitx5-mozc to 2.30.5544.102 with the bazel build system.

I didn't choose the latest version (2.30.5595.102) because it uses the new local.bzl feature that is only available in bazel >7.2 (see: bazelbuild/bazel#22612), so that's blocked until #338264 is merged.

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

musjj added 4 commits October 5, 2024 20:02
This package can be consumed and used by not just ibus, but by many
other packages/tools, so it should have a more generic name.
mozc-ut is not an ibus-exclusive package, so it should live in a higher
namespace
@musjj musjj requested a review from alyssais as a code owner October 5, 2024 14:26
@github-actions github-actions Bot added 6.topic: lib The Nixpkgs function library 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Oct 5, 2024
@ofborg ofborg Bot added the 8.has: package (new) This PR adds a new package label Oct 5, 2024
@ofborg ofborg Bot requested a review from gebner October 5, 2024 15:11
@ofborg ofborg Bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Oct 5, 2024
Comment on lines +131 to +136
asl20 # abseil-cpp
bsd3 # mozc, breakpad, gtest, gyp, japanese-usage-dictionary, protobuf
mit # wil
naist-2003 # IPAdic
publicDomain # src/data/test/stress_test, Okinawa dictionary
unicode-30 # src/data/unicode, breakpad
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Because of bazel we cannot unvendor things, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comment thread pkgs/top-level/all-packages.nix Outdated
Copy link
Copy Markdown
Member

@stephen-huan stephen-huan left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this PR over more than a year (!)! I don't know bazel and fcitx that much so I'll just comment on the ibus integration. The integration here is tight and exactly what I imagined when I made my comments.

The old PR #251706 packaged google/mozc with the bazelTargets

bazelTargets = [
  "server:mozc_server"
  "gui/tool:mozc_tool"
];

I proposed in #251706 (comment) to remove this in favor of re-using ibus-engines.mozc.

It seems possible to expose one mozc derivation packaging google/mozc and use this for both ibus-engines.mozc and fcitx5-mozc to prevent redundancy and simplify maintenance.

but was shot down by @mikunimaru in #251706 (comment) and #251706 (comment).

Also you mentioned in #251706 (comment)

As for the ibus stuff, I'm not really sure since I never used it. But theoretically it should be possible to share the mozc package between it, by making the right overrides to the bazelTargets.

That seems to have happened in this PR, now that mozc is no longer being re-packaged (the first two commits rename ibus-mozc to mozc and expose mozc-ut at the top level). But the bazelTargets of current mozc is different than the mozc packaged in the old PR.

bazelTargets = [ "package" ];

What changed in the derivation to not require overriding bazelTargets? And what do you think about @mikunimaru's comments about the re-use?

@musjj
Copy link
Copy Markdown
Contributor Author

musjj commented Oct 5, 2024

The new package by @pineapplehunter uses the package target which bundles in other targets, including fcitx5, ibus and emacs:

mozc/src/unix/BUILD.bazel

genrule(
    name = "package",
    srcs = [
        ":icons",
        "//gui/tool:mozc_tool",
        "//server:mozc_server",
        "//unix/emacs:mozc_emacs_helper",
        "//unix/ibus:gen_mozc_xml",
        "//unix/ibus:ibus_mozc",
        "//unix/emacs:mozc.el",
        "//renderer/qt:mozc_renderer",
    ],

So it's just a lot more convenient now.

As for @mikunimaru's concerns, the original PR was actually built using google/mozc from the start (for mozc_server). I've daily-driven it since then and I have yet to see any issues, so incompatibility shouldn't be a concern. It's also how the AUR package is built, which is what I based my PR on.

Copy link
Copy Markdown
Member

@stephen-huan stephen-huan left a comment

Choose a reason for hiding this comment

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

Haven't tested fcitx, but the integration with ibus-mozc and mozc-ut looks good!

in
buildBazelPackage rec {
pname = "ibus-mozc";
pname = "mozc";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for changing this! I forgot about it.

Comment thread pkgs/tools/inputmethods/fcitx5/fcitx5-mozc.nix Outdated
Comment thread pkgs/tools/inputmethods/fcitx5/fcitx5-mozc.nix
Comment thread pkgs/tools/inputmethods/fcitx5/fcitx5-mozc.nix
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Oct 7, 2024
Comment thread pkgs/top-level/all-packages.nix Outdated
@ofborg ofborg Bot requested a review from pineapplehunter October 8, 2024 16:18
Copy link
Copy Markdown
Contributor

@pineapplehunter pineapplehunter left a comment

Choose a reason for hiding this comment

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

LGTM!


Result of nixpkgs-review pr 346680 run on x86_64-linux 1

1 package blacklisted:
  • nixos-install-tools
12 packages built:
  • emacsPackages.ac-mozc
  • emacsPackages.mozc
  • emacsPackages.mozc-cand-posframe
  • emacsPackages.mozc-im
  • emacsPackages.mozc-popup
  • emacsPackages.mozc-temp
  • fcitx5-mozc
  • fcitx5-mozc-ut
  • ibus-engines.mozc
  • ibus-engines.mozc-ut
  • nix-init
  • nixpkgs-manual

Comment thread pkgs/top-level/all-packages.nix
@ofborg ofborg Bot requested a review from pineapplehunter October 8, 2024 17:18
@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Oct 9, 2024
@mikunimaru
Copy link
Copy Markdown

I think neither fcitx5-mozc nor fcitx5-mozc-ut use the UT dictionary.
Is it confirmed to work?
In the PKGBUILD, fcitx5-mozc-ut depends on the original mozc, so I think this means that even if you change mozc to mozc-ut, it has nothing to do with the dictionary.
I apologize if it was just a misunderstanding.

@mikunimaru
Copy link
Copy Markdown

I have confirmed that fcitx5-mozc and fcitx5-mozc-ut are correctly using different dictionaries, so I think this pull request should be approved.

@mikunimaru
Copy link
Copy Markdown

In addition, even if a problem occurs in the future due to a version difference between google/mozc and fcitx/mozc, it can be resolved by simply changing the code on the nixpkgs side, without requiring users to go to the trouble of modifying nix files.
This resolves my initial concerns.

Also, from my personal research, I found that the differences between fcitx/mozc and google/mozc have not changed since 2015, so even if there is a version difference between google/mozc and fcitx/mozc, it seems unlikely that problems will occur immediately.
google/mozc@master...fcitx:mozc:fcitx

Overall, I think this is a great pull request.

@wegank wegank added 12.approvals: 3+ This PR was reviewed and approved by three or more persons. and removed 12.approvals: 2 This PR was reviewed and approved by two persons. labels Oct 11, 2024
Comment thread pkgs/by-name/fc/fcitx5-mozc/package.nix Outdated
@ofborg ofborg Bot requested a review from pineapplehunter October 26, 2024 13:23
Co-authored-by: h7x4 <h7x4@nani.wtf>
@h7x4
Copy link
Copy Markdown
Member

h7x4 commented Oct 27, 2024

I am not able to reproduce the test error locally, and the assertion error is not very helpful. I guess it is because of timing issues and not because of the package itself. I will merge this PR in the current state and open another PR to improve the assertion error and maybe the test as well.

Thank you so much for the great work!

@h7x4 h7x4 merged commit cabfffd into NixOS:master Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: lib The Nixpkgs function library 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 12.approvals: 3+ This PR was reviewed and approved by three or more persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants