fcitx5-mozc: 2.26.4220.102 -> 2.30.5544.102#346680
Conversation
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
| 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 |
There was a problem hiding this comment.
Because of bazel we cannot unvendor things, right?
There was a problem hiding this comment.
It's mostly from the submodules in here: https://github.com/fcitx/mozc/tree/ace314567109cbc30dc336fb27844dacff782dd9/src/third_party.
General info about the license: https://github.com/fcitx/mozc/tree/master?tab=readme-ov-file#license
stephen-huan
left a comment
There was a problem hiding this comment.
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
mozcderivation packaging google/mozc and use this for bothibus-engines.mozcandfcitx5-mozcto 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.
nixpkgs/pkgs/by-name/mo/mozc/package.nix
Line 63 in bc947f5
What changed in the derivation to not require overriding bazelTargets? And what do you think about @mikunimaru's comments about the re-use?
|
The new package by @pineapplehunter uses the 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 |
stephen-huan
left a comment
There was a problem hiding this comment.
Haven't tested fcitx, but the integration with ibus-mozc and mozc-ut looks good!
| in | ||
| buildBazelPackage rec { | ||
| pname = "ibus-mozc"; | ||
| pname = "mozc"; |
There was a problem hiding this comment.
Thanks for changing this! I forgot about it.
pineapplehunter
left a comment
There was a problem hiding this comment.
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
|
I think neither fcitx5-mozc nor fcitx5-mozc-ut use the UT dictionary. |
|
I have confirmed that fcitx5-mozc and fcitx5-mozc-ut are correctly using different dictionaries, so I think this pull request should be approved. |
|
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. 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. Overall, I think this is a great pull request. |
Co-authored-by: h7x4 <h7x4@nani.wtf>
00db16a to
65648a8
Compare
|
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! |
Upgrades fcitx5-mozc to
2.30.5544.102with the bazel build system.I didn't choose the latest version (
2.30.5595.102) because it uses the newlocal.bzlfeature that is only available in bazel >7.2 (see: bazelbuild/bazel#22612), so that's blocked until #338264 is merged.Things done
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/)Add a 👍 reaction to pull requests you find important.