Non-public Abseil API is used for CLI parsing

musicinmybrain commented 1 year ago

Description

Non-public APIs from the absl::flags_internal namespace are used for command-line option parsing.

https://github.com/google/mozc/blob/88d5ef3ff738c9574112c0afba1d2f982ceaa461/src/base/init_mozc.cc#L88-L94

Since this interface is removed in the latest abseil-cpp release, this blocks upgrading Abseil.

Steps to reproduce

Steps to reproduce the behavior:

Examine the lines in the Description section.
Observe that non-public APIs are used.

Expected behavior

Only public APIs are used; upgrading abseil-cpp to the latest LTS release is possible.

Actual behavior

Some non-public APIs are used; upgrading abseil-cpp to the latest LTS release is not possible, resulting in an error like:

../../base/init_mozc.cc: In function ‘void mozc::{anonymous}::ParseCommandLineFl
ags(int, char**)’:
../../base/init_mozc.cc:90:29: error: ‘absl::lts_20230802::flags_internal::ArgvListAction’ has not been declared
   90 |       absl::flags_internal::ArgvListAction::kRemoveParsedArgs,
      |                             ^~~~~~~~~~~~~~

Screenshots

N/A

Version or commit-id

Current master 88d5ef3ff738c9574112c0afba1d2f982ceaa461.

Or, 2.29.5111.102.

Environment

OS: Fedora Linux Rawhide; but this is not a platform-dependent issue
IMF (for Linux) [e.g. Ibus, Fcitx, etc]: N/A
Related Applications [e.g. Chromium 109, gedit, etc]: N/A

Investigations

Whether this issue happens on both Ibus and other IMF (e.g. Fcitx, uim).
- N/A
Whether this issue happens on other IMEs (e.g. Anthy, SKK).
- N/A
What applications this issue happens on (e.g. Chromium, gedit).
- N/A
What applications this issue does not happen on (e.g. Chromium, gedit).
- N/A
(optional) What versions or commit-ids this issue did not happen on (e.g. Mozc-2.28.4960.100+24.11.oss).
- N/A

Additional context

I discovered this problem while attempting to upgrade the abseil-cpp package in Fedora Linux, where a patch is used to build mozc with the system copy of Abseil.

https://github.com/abseil/abseil-cpp/blob/20230802.0/absl/flags/internal/parse.h#L45-L53

// This is not a public interface. This interface exists to expose the ability
// to change help output stream in case of parsing errors. This is used by
// internal unit tests to validate expected outputs.
// When this was written, `EXPECT_EXIT` only supported matchers on stderr,
// but not on stdout.
std::vector<char*> ParseCommandLineImpl(
    int argc, char* argv[], UsageFlagsAction usage_flag_action,
    OnUndefinedFlag undef_flag_action,
    std::ostream& error_help_output = std::cout);

hiroyuki-komatsu commented 1 year ago

Hi musicinmybrain, Thank you for the feedback.

We are going to update the Abseil version to 20230802.0 in near future. However the latest release of Protobuf does not support the latest Abseil yet. It's a blocking issue for us.

https://github.com/protocolbuffers/protobuf/pull/13534

So we start updating the Abseil version after the new Protobuf (e.g. 24.1) is released. Meanwhile, I hope the following change works on your side. We'd also like to recommend to use a static link without the patch.

void ParseCommandLineFlags(int argc, char **argv) {
  absl::flags_internal::ParseCommandLineImpl(
      argc, argv,
      // Abseil 20230802.0 does not use ArgvListAction
      // absl::flags_internal::ArgvListAction::kRemoveParsedArgs,
      // Suppress help messages invoked by --help and others.
      // Use UsageFlagsAction::kHandleUsage to enable it.
      absl::flags_internal::UsageFlagsAction::kIgnoreUsage,
      absl::flags_internal::OnUndefinedFlag::kIgnoreUndefined);
}

Thanks,

yukawa commented 1 year ago

https://github.com/protocolbuffers/protobuf/pull/13534 So we start updating the Abseil version after the new Protobuf (e.g. 24.1) is released. Meanwhile, I hope the following change works on your side. We'd also like to recommend to use a static link without the patch.

Just fyi, the above change in protobuf 24.x branch got reverted at https://github.com/protocolbuffers/protobuf/pull/13597. Whether protobuf 24.x is going to be related with Abseil 20230802.0 looks to be unclear at this moment.

musicinmybrain commented 1 year ago

So we start updating the Abseil version after the new Protobuf (e.g. 24.1) is released. Meanwhile, I hope the following change works on your side. We'd also like to recommend to use a static link without the patch.

The patch works perfectly; thank you! For our purposes, the benefits of linking against system-wide shared libraries outweigh the costs of carrying the patch.

We are going to update the Abseil version to 20230802.0 in near future. However the latest release of Protobuf does not support the latest Abseil yet. It's a blocking issue for us.

Just fyi, the above change in protobuf 24.x branch got reverted at protocolbuffers/protobuf#13597. Whether protobuf 24.x is going to be related with Abseil 20230802.0 looks to be unclear at this moment.

Thank you both for mentioning this. We aren’t blocked from updating Abseil for Fedora 39 (largely because we haven’t been able to make the transition to protobuf v4, 22.x and later, yet), but these links will save some time down the road.

hiroyuki-komatsu commented 1 year ago

cad4064c8884eb711e0e19b4b79d2ff5610823dc fixes the build error with Abseil 20230802.0 by adding the version check.

Thank you for your feedback.

yukawa commented 1 year ago

The patch works perfectly; thank you! For our purposes, the benefits of linking against system-wide shared libraries outweigh the costs of carrying the patch.

@musicinmybrain, just fyi, it seems that Abseil exposes different ABIs depending on what C++ language level is chosen when building it. See the following discussions for instance.

So it's not only about a classical "diamond dependency problem" (*) any more but also a "diamond C++ language level problem". Just as a heads up Mozc will soon start to using C++20 (#783), and what Mozc would expect are abseil built with -std=c++20 and protobuf built with -std=c++20. Perhaps you may end up having to create things like libabsl_base_cpp14.so.2308.0.0, libabsl_base_cpp17.so.2308.0.0 and libabsl_base_cpp20.so.2308.0.0.

(*) https://abseil.io/resources/swe-book/html/ch21.html

The diamond dependency problem, and other forms of conflicting requirements, require at least three layers of dependency, as demonstrated in Figure 21-1.

Figure 21-1. The diamond dependency problem

musicinmybrain commented 1 year ago

@musicinmybrain, just fyi, it seems that Abseil exposes different ABIs depending on what C++ language level is chosen when building it.

Thanks for pointing that out. I’m aware of the significant ABI difference from C++14 to C++17; we handled it by making sure all packages that depend on abseil-cpp are compiled as C++17 or later. So far, we haven’t encountered any problems linking packages built as C++20 to abseil-cpp built as C++17, but it’s possible we are just living dangerously and have yet to encounter the problems we are creating. It’s possible to bump everything up to C++20, but once we move over to protobuf v4 (22.x, 23.x, ...) and the generated protobuf bindings start to depend on Abseil, the number of packages impacted by such a change will be much larger, so I hope this won’t be necessary. It would still probably be a more workable approach than having multiple versions of the Abseil shared libraries with different ABIs that can’t be linked together.

google / mozc