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

Disable check for Objective C compiler (AC_PROG_OBJC) on non-macOS systems #9392

Closed
Arfrever opened this issue Jan 8, 2022 · 10 comments · Fixed by #9543
Closed

Disable check for Objective C compiler (AC_PROG_OBJC) on non-macOS systems #9392

Arfrever opened this issue Jan 8, 2022 · 10 comments · Fixed by #9543

Comments

@Arfrever
Copy link
Contributor

Arfrever commented Jan 8, 2022

Autotools build system currently unconditionally checks for Objective C compiler:

AC_PROG_OBJC

However later there is:

protobuf/configure.ac

Lines 217 to 224 in 0ac74b8

# Enable ObjC support for conformance directory on OS X.
OBJC_CONFORMANCE_TEST=0
case "$target_os" in
darwin*)
OBJC_CONFORMANCE_TEST=1
;;
esac
AM_CONDITIONAL([OBJC_CONFORMANCE_TEST], [test $OBJC_CONFORMANCE_TEST = 1])

OBJC_CONFORMANCE_TEST is used in conformance/Makefile.am.
The only usage of Objective C compiler is for compiling source files which are used only when OBJC_CONFORMANCE_TEST is true.

This means that aforementioned check for Objective C compiler is unnecessary on non-macOS systems (when $target_os != darwin*).

Fix:

--- configure.ac
+++ configure.ac
@@ -80,7 +80,7 @@
 ACX_USE_SYSTEM_EXTENSIONS
 m4_ifdef([AM_PROG_AR], [AM_PROG_AR])
 AM_CONDITIONAL(GCC, test "$GCC" = yes)   # let the Makefile know if we're gcc
-AC_PROG_OBJC
+AS_CASE([$target_os], [darwin*], [AC_PROG_OBJC], [AM_CONDITIONAL([am__fastdepOBJC], [false])])
 
 # test_util.cc takes forever to compile with GCC and optimization turned on.
 AC_MSG_CHECKING([C++ compiler flags...])
@elharo
Copy link
Contributor

elharo commented Jan 11, 2022

Does this break anything?

@Arfrever
Copy link
Contributor Author

Arfrever commented Jan 11, 2022

We have downstream report of user ("targeting Gentoo Linux and Chromium OS") complaining about check for Objective-C compiler selection: https://bugs.gentoo.org/830584

@thesamesam
Copy link

cc @10ne1

@10ne1
Copy link

10ne1 commented Jan 14, 2022

Thanks for reporting this. It is just a minor annoyance for us (ChromiumOS based on Gentoo) and it would be nice to avoid having to set an OBJC compiler if it is not necessary.

For context, we are interested in building packages only with LLVM tools/Clang and OBJC defaults to gcc. We intentionally block GCC/binutils, but we still need to keep it around for glibc and a very small handful of other packages which can't be built with LLVM/Clang (yet! it will happen eventually heh).

Again this is only a minor annoyance because we can explicitly set OBJC to Clang, but it would be nice to not have to do so. Thanks again!

@thomasvl
Copy link
Contributor

I'm not likely to get the to look at this in the near future, if someone wants to submit a patch, great.

@thomasvl thomasvl removed their assignment Jan 26, 2022
@haberman
Copy link
Member

haberman commented Feb 4, 2022

Can you migrate to use the CMake build instead?

Our autotools build is on its last legs, and I anticipate we will be deprecating it before too long in favor of CMake.

@Arfrever
Copy link
Contributor Author

Arfrever commented Feb 4, 2022

Protobuf's CMake build system is still in poorer state.

Until commit a9cf69a 3 days ago, CMake build system was not setting sonames of libraries properly, which means that they were ABI-incompatible with libraries created by Autotools build system.
And that commit fixed only 1/3 of that problem (as explained in https://github.com/protocolbuffers/protobuf/issues/8635)...

https://github.com/protocolbuffers/protobuf/issues/6113 (also important for ABI compatibility) and possibly other bugs still remain.
cmake/README.md file still says:

This directory contains CMake files that can be used to build protobuf with MSVC on Windows.

Which implies that it is not to be used on non-Windows systems.

@thomasvl
Copy link
Contributor

@Arfrever looks like you might have a have the start of a patch already? Is this something you could put up a PR for? And while I'm asking, what's the AM_CONDITIONAL block in that diff needed for?

@thomasvl
Copy link
Contributor

Testing out a minimal version of the above diff to see if it atleast works on the CI systems.

thomasvl added a commit to thomasvl/protobuf that referenced this issue Feb 24, 2022
thomasvl added a commit to thomasvl/protobuf that referenced this issue Feb 24, 2022
@thomasvl
Copy link
Contributor

Testing out a minimal version of the above diff to see if it atleast works on the CI systems.

Looks like something trips up on am__fastdepOBJC, so I guess that's why it ends up being forced false. Testing adding that in also.

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

Successfully merging a pull request may close this issue.

7 participants