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

mozc: init at unstable-2024-02-09, fcitx5-mozc: 2.26.4220.102 -> unstable-2024-02-09 #251706

Closed
wants to merge 24 commits into from

Conversation

musjj
Copy link
Contributor

@musjj musjj commented Aug 27, 2023

Description of changes

This upgrades mozc to the latest version (the last version was over 2 years old and suffers from some issues). Consequently, with the deprecation of the GYP I chose to move to the new Bazel build system.

I'm really unfamiliar with Bazel, so I'd really appreciate it if anyone with experience can help check if everything's alright, especially in terms of reproducibility.

I also choose to modularize mozc as a library, so that other packagers can re-use it if they want to.

AUR packages I used for reference:

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

Copy link
Member

@Vonfry Vonfry left a comment

Choose a reason for hiding this comment

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

I'm not use mozc. The fcitx5-mozc diff is LGTM.

pkgs/tools/inputmethods/fcitx5/fcitx5-mozc.nix Outdated Show resolved Hide resolved
@musjj musjj force-pushed the fcitx-mozc-bazel branch 6 times, most recently from ea81e84 to bc1b7ab Compare August 27, 2023 15:47
@musjj musjj force-pushed the fcitx-mozc-bazel branch 3 times, most recently from 77604d5 to 8279c16 Compare September 6, 2023 09:10
@musjj
Copy link
Contributor Author

musjj commented Sep 6, 2023

I'm struggling to keep the dependency hash stable. I'm not really sure what's causing the inconsistency though.

I first thought it was caused by the zip code archives, so I mirrored the archive to a tracked git repository to ensure that it's reproducible, but it still breaks anyways.

Anyone have any ideas?
@abbradar Sorry for pinging, but I heard that you're good with Bazel :).

@musjj musjj force-pushed the fcitx-mozc-bazel branch 4 times, most recently from 9fea5c6 to 8ebbe81 Compare September 6, 2023 15:05
Copy link
Contributor

@carpinchomug carpinchomug left a comment

Choose a reason for hiding this comment

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

Thank you for your effort to update mozc! I've been trying to do this myself as well, but couldn't figure out how to package a Bazel project.

Unfortunately, I'm not familiar enough with either bazel or mozc to offer help with the hash issue, but I do have a suggestion/request.

mozccan optionally be built with mozc_emacs_helper, which is a helper programs that enables integration of mozc with Emacs.

Currently, mozc_emacs_helper can be built and installed with ibus-engines.mozc. But now that you factored out mozc as a standalone package, I think it makes more sense for mozc_emacs_helper to be provided by this new mozc package rather than by ibus-engines.mozc after this PR is merged.

Reading the build instructions in the mozc repo, adding these lines should build mozc with mozc_emacs_helper. I tested this on my laptop and it seems to be working correctly.

What do you think of these changes?

PS: I have never submitted a review on GitHub before, I'm sorry if I'm not following the right format.

Comment on lines +141 to +146
bazelTargets = [
"server:mozc_server"
"gui/tool:mozc_tool"
];
Copy link
Contributor

@carpinchomug carpinchomug Sep 6, 2023

Choose a reason for hiding this comment

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

Suggested change
bazelTargets = [
"server:mozc_server"
"gui/tool:mozc_tool"
];
bazelTargets = [
"server:mozc_server"
"gui/tool:mozc_tool"
"unix/emacs:mozc_emacs_helper"
];

Add mozc_emacs_helper to build targets.

buildAttrs = {
postPatch = ''
sed -ri -e "s|^(LINUX_MOZC_SERVER_DIR = ).+|\1\"$out/lib/mozc\"|" mozc/src/config.bzl

Copy link
Contributor

@carpinchomug carpinchomug Sep 6, 2023

Choose a reason for hiding this comment

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

Suggested change
sed -ri -e "s|^(EMACS_MOZC_HELPER_DIR = ).+|\1\"$out/bin\"|" mozc/src/config.bzl

Fix the install path for mozc_emacs_helper.

mkdir -p $out/lib/mozc
cp bazel-bin/server/mozc_server $out/lib/mozc/mozc_server
cp bazel-bin/gui/tool/mozc_tool $out/lib/mozc/mozc_tool

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mkdir -p $out/bin
cp bazel-bin/unix/emacs/mozc_emacs_helper $out/bin/mozc_emacs_helper

Install mozc_emacs_helper.

@musjj
Copy link
Contributor Author

musjj commented Sep 6, 2023

@carpinchomug

Thanks for the suggestion, but I think this should be a separate package. The point of the mozc package is solely to provide the mozc_{server,tool} library for other packages.

If you're looking to make a package for this, you can look at fcitx5-mozc.nix in this PR and this AUR package for reference.

@h7x4
Copy link
Member

h7x4 commented Sep 6, 2023

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

2 packages built:
  • fcitx5-mozc
  • mozc

@musjj musjj force-pushed the fcitx-mozc-bazel branch 2 times, most recently from 9bf4f7f to 79c81c7 Compare September 11, 2023 10:50
@musjj
Copy link
Contributor Author

musjj commented Sep 11, 2024

I rebased and updated the packages. I also made a new repository to solve the jawiki issue: https://github.com/musjj/jawiki-archive.

@stephen-huan

Sorry for the late reply.

As suggested in #296035 (comment) I think it is cleaner to split the dictionary into a separate derivation (e.g. mozcdic-ut).

How would this work? Should it be split into 3 different packages like:

  • mozcdic-ut
  • mozc
  • mozc-ut

What do you think?

I might wait out and see if @pineapplehunter's #314248 gets merged since it seems like it fulfills the concerns around modularity better.

@stephen-huan
Copy link
Member

The split into mozcdic-ut, mozc, mozc-ut sounds good. #314248 is actually more fine-grain than that (it further splits mozcdic-ut into its requisite dictionaries, but this is mostly for ease of auto-update). I'm not sure whether it will be merged soon though, it's still marked as draft.

@h7x4
Copy link
Member

h7x4 commented Oct 4, 2024

@musjj Now that #314248 is merged, are you up to the task of rewriting this PR to have fcitx5-mozc use the newly added sources (or alternatively open new PR if you'd like)? I realize that I'm asking you for a whole new chunk of work, so feel free to say no if you don't want to :)

@musjj
Copy link
Contributor Author

musjj commented Oct 5, 2024

Yeah, I think it's better to open a new PR.

Closed in favor of: #346680

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.