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

Non-public Abseil API is used for CLI parsing #790

Closed
musicinmybrain opened this issue Aug 14, 2023 · 6 comments
Closed

Non-public Abseil API is used for CLI parsing #790

musicinmybrain opened this issue Aug 14, 2023 · 6 comments

Comments

@musicinmybrain
Copy link

Description

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

absl::flags_internal::ParseCommandLineImpl(
argc, argv,
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);

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

Steps to reproduce

Steps to reproduce the behavior:

  1. Examine the lines in the Description section.
  2. 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 88d5ef3.

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
Copy link
Collaborator

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.

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
Copy link
Collaborator

yukawa commented Aug 21, 2023

protocolbuffers/protobuf#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 protocolbuffers/protobuf#13597. Whether protobuf 24.x is going to be related with Abseil 20230802.0 looks to be unclear at this moment.

@musicinmybrain
Copy link
Author

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 added a commit that referenced this issue Sep 1, 2023
* Added the check of the Abseil version to the arguments of ParseCommandLineImpl.
* #790

#codehealth

PiperOrigin-RevId: 561867167
@hiroyuki-komatsu
Copy link
Collaborator

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

Thank you for your feedback.

@yukawa
Copy link
Collaborator

yukawa commented Sep 4, 2023

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.

The diamond dependency problem
Figure 21-1. The diamond dependency problem

@musicinmybrain
Copy link
Author

@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.

coooooooozy pushed a commit to coooooooozy/mozc that referenced this issue Nov 26, 2023
* Added the check of the Abseil version to the arguments of ParseCommandLineImpl.
* google#790

#codehealth

PiperOrigin-RevId: 561867167
kenhys added a commit to kenhys/mozc that referenced this issue Oct 16, 2024
Origin: google@cad4064
Description: Fix the compile error of ParseCommandLineFlags with Abseil LTS 20230802.
  * Added the check of the Abseil version to the arguments of ParseCommandLineImpl.
  * google#790
  #codehealth
  PiperOrigin-RevId: 561867167
Author: Hiroyuki Komatsu <[email protected]>

Released in 2.29.5374.102

Signed-off-by: Kentaro Hayashi <[email protected]>
kenhys added a commit to kenhys/mozc that referenced this issue Nov 13, 2024
Origin: google@cad4064
Description: Fix the compile error of ParseCommandLineFlags with Abseil LTS 20230802.
  * Added the check of the Abseil version to the arguments of ParseCommandLineImpl.
  * google#790
  #codehealth
  PiperOrigin-RevId: 561867167
Author: Hiroyuki Komatsu <[email protected]>

Released in 2.29.5374.102

Signed-off-by: Kentaro Hayashi <[email protected]>
kenhys added a commit to kenhys/mozc that referenced this issue Nov 14, 2024
Origin: google@cad4064
Description: Fix the compile error of ParseCommandLineFlags with Abseil LTS 20230802.
  * Added the check of the Abseil version to the arguments of ParseCommandLineImpl.
  * google#790
  #codehealth
  PiperOrigin-RevId: 561867167
Author: Hiroyuki Komatsu <[email protected]>

Released in 2.29.5374.102

Signed-off-by: Kentaro Hayashi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants