-
-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
fcitx5-mozc: 2.26.4220.102 -> 2.30.5544.102 #346680
base: master
Are you sure you want to change the base?
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.
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?
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.
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
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.
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 bothibus-engines.mozc
andfcitx5-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.
nixpkgs/pkgs/by-name/mo/mozc/package.nix
Line 63 in bc947f5
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?
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 |
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.
Haven't tested fcitx, but the integration with ibus-mozc and mozc-ut looks good!
@@ -17,7 +17,7 @@ let | |||
ut-dictionary = merge-ut-dictionaries.override { inherit dictionaries; }; | |||
in | |||
buildBazelPackage rec { | |||
pname = "ibus-mozc"; | |||
pname = "mozc"; |
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.
Thanks for changing this! I forgot about it.
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.
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
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 newlocal.bzl
feature 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 = 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/
)Add a 👍 reaction to pull requests you find important.