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

Prefer /usr/bin/clang as default CC on darwin #10823

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kateinoigakukun
Copy link
Member

/usr/bin/clang is always available as long as Command Line Tools or Xcode is installed. Prefer the absolute path to avoid PATH lookup, which may lead to unaligned toolset selection.

This change makes toolset selection more robust by:

  1. Use /usr/bin/clang as default CC on darwin.
  2. Prefer other tools next to CC if selected CC is absolute and the tool is not explicitly specified.

configure.ac Outdated Show resolved Hide resolved
Comment on lines +184 to +200
[darwin*], [
# /usr/bin/clang is always available as long as Command Line Tools or Xcode is installed.
# Prefer absolute path to avoid PATH lookup, which may lead to unaligned tool selection.
rb_darwin_default_CC=/usr/bin/clang
AC_MSG_CHECKING([for darwin default CC $rb_darwin_default_CC])
AS_IF([test -z "$CC" -a -x "$rb_darwin_default_CC"],
[
AC_MSG_RESULT([yes])
CC=$rb_darwin_default_CC
],
[
AC_MSG_RESULT([no])
# Fall back to PATH lookup just in case the /usr/bin/clang is not
# available for some reason.
AC_CHECK_TOOLS([CC], [clang cc gcc c99])
]
)
Copy link
Member

Choose a reason for hiding this comment

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

What about expand rb_CC absolute path, only if the found CC is Xcode?

    AS_IF([$rb_CC -E -dM -xc - < /dev/null | grep -F __apple_build_version__ > /dev/null],
          [llvm_prefix= rb_CC=`command -v $rb_CC`], [llvm_prefix=llvm-])

Copy link
Member Author

@kateinoigakukun kateinoigakukun May 23, 2024

Choose a reason for hiding this comment

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

What we are trying to solve is unaligned toolset selection in the situation where PATH contains mixed toolchains (e.g. LLVM from Homebrew, Conda toolchain?, and Xcode).

e.g.

$ ls /path/to/conda/bin
clang # but no nm
$ ls /usr/bin
clang nm ...
$ export PATH=/path/to/conda:/usr/bin

If we continue looking up CC from PATH and align toolset only when CC comes from Xcode, the problem won't be mitigated for the above situation.
That's why I prefer fixed /usr/bin/clang as the default decision.

Copy link

@rubyFeedback rubyFeedback May 23, 2024

Choose a reason for hiding this comment

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

Not disagreeing with you as such, but it should be pointed out that at the least on Linux, the default assumption is that the entries in $PATH coming first, have higher precedence. So, for instance, if a user would have a PATH that first has /opt/bin/, and /usr/bin/ as second entry, then /opt/bin/ should be searched first. This will probably happen only very, very rarely, so it does not invalidate your statement, but I wanted to mention it in regards to (possible) user's expectations.

If we continue looking up CC from PATH and align toolset only when CC comes from Xcode, the problem won't be mitigated for the above situation.

Right, but it should also pointed out that this appears to be an incorrect assumption by conda then, if it assumes that the bin/ it searches for must be in the same directory (e. g. assuming /usr/bin/). My clang binary, for instance, is under /home/Programs/Llvm/18.1.2/bin/, for instance. (I follow a symlink-version approach similar to GoboLinux, but I also only use /usr/bin/ and just update the current symlink in use, for all programs on my linux machine, just as info; obviously darwin follows another scheme.)

Copy link
Member

Choose a reason for hiding this comment

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

I think we should abort in that case, if not [test -x "${rb_prog}"], rather than searching from $PATH.

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense to me. It sounds well consistent behavior.

Copy link
Member

Choose a reason for hiding this comment

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

I think rb_darwin_default_CC is not needed anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, do you mean we should find CC from PATH instead of having the default value?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, only for Xcode clang, as commented above.

configure.ac Outdated Show resolved Hide resolved
`/usr/bin/clang` is always available as long as Command Line Tools
or Xcode is installed. Prefer the absolute path to avoid PATH lookup,
which may lead to unaligned toolset selection.

This change makes toolset selection more robust by:
1. Use `/usr/bin/clang` as default CC on darwin.
2. Prefer other tools next to `CC` if selected CC is absolute and
   the tool is not explicitly specified.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants