-
-
Notifications
You must be signed in to change notification settings - Fork 638
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
add a --toolchain-variant option to select the compiler for C/C++ #6800
Changes from 3 commits
e78a483
a799016
71ca371
bf9e9a7
363c940
1de3799
6ed4110
c4b4f28
ebeb2fa
282e9db
b53b5c0
7ee1dd9
1b9804e
8375a62
b32d643
1a2108d
15632fe
0e3c3e8
0703beb
6af5aca
9a25d32
64e3e54
5473093
55e6908
3cd803a
408f4cd
a3b4339
45e4c7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,9 +6,9 @@ | |
|
||
import os | ||
from abc import abstractproperty | ||
from builtins import object | ||
|
||
from pants.engine.rules import SingletonRule | ||
from pants.util.meta import AbstractClass | ||
from pants.util.objects import datatype | ||
from pants.util.osutil import all_normalized_os_names, get_normalized_os_name | ||
from pants.util.strutil import create_path_env_var | ||
|
@@ -42,34 +42,54 @@ def resolve_platform_specific(self, platform_specific_funs): | |
return fun_for_platform() | ||
|
||
|
||
class Executable(object): | ||
# NB: @abstractproperty requires inheriting from AbstractClass to work! | ||
class Executable(AbstractClass): | ||
|
||
@abstractproperty | ||
def path_entries(self): | ||
"""A list of directory paths containing this executable, to be used in a subprocess's PATH. | ||
|
||
This may be multiple directories, e.g. if the main executable program invokes any subprocesses. | ||
|
||
:rtype: list of str | ||
""" | ||
|
||
@abstractproperty | ||
def exe_filename(self): | ||
"""The "entry point" -- which file to invoke when PATH is set to `path_entries()`. | ||
|
||
:rtype: str | ||
""" | ||
|
||
# TODO(#???): rename this to 'runtime_library_dirs'! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please fill in the issue link or else kill the unfinished #??? thought. A todo that itself is in a half-done state is just too much. Maybe fine when you're hacking locally, but you really should clean things up like this before posting for review. You may be making an effort at this though and just missed this one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it was possible to run testing through the travis shards before pushing a commit, I would never have any such comments. I'll grant that magit in emacs makes it easy to stage hunks of files and not the whole diff, but since bootstrapping pants when running in a docker image takes a very long time, the only method of iteration I can see is to abuse travis runs on each commit. I feel that this has continually led to the assumption that I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the |
||
@abstractproperty | ||
def library_dirs(self): | ||
"""Directories containing shared libraries that must be on the runtime library search path. | ||
|
||
Note: this is for libraries needed for the current Executable to run -- see LinkerMixin below | ||
for libraries that are needed at link time.""" | ||
for libraries that are needed at link time. | ||
|
||
@abstractproperty | ||
def exe_filename(self): | ||
"""The "entry point" -- which file to invoke when PATH is set to `path_entries()`.""" | ||
:rtype: list of str | ||
""" | ||
|
||
@property | ||
def extra_args(self): | ||
"""Additional arguments used when invoking this Executable. | ||
|
||
These are typically placed before the invocation-specific command line arguments. | ||
|
||
:rtype: list of str | ||
""" | ||
return [] | ||
|
||
_platform = Platform.create() | ||
|
||
@property | ||
def as_invocation_environment_dict(self): | ||
"""A dict to use as this Executable's execution environment. | ||
|
||
:rtype: dict of string -> string | ||
""" | ||
lib_env_var = self._platform.resolve_platform_specific({ | ||
'darwin': lambda: 'DYLD_LIBRARY_PATH', | ||
'linux': lambda: 'LD_LIBRARY_PATH', | ||
|
@@ -92,13 +112,18 @@ class LinkerMixin(Executable): | |
|
||
@abstractproperty | ||
def linking_library_dirs(self): | ||
"""Directories to search for libraries needed at link time.""" | ||
"""Directories to search for libraries needed at link time. | ||
|
||
:rtype: list of str | ||
""" | ||
|
||
@abstractproperty | ||
jsirois marked this conversation as resolved.
Show resolved
Hide resolved
|
||
def extra_object_files(self): | ||
"""A list of object files required to perform a successful link. | ||
|
||
This includes crti.o from libc for gcc on Linux, for example. | ||
|
||
:rtype: list of str | ||
""" | ||
jsirois marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
@property | ||
|
@@ -131,7 +156,10 @@ class CompilerMixin(Executable): | |
|
||
@abstractproperty | ||
def include_dirs(self): | ||
"""Directories to search for header files to #include during compilation.""" | ||
"""Directories to search for header files to #include during compilation. | ||
|
||
:rtype: list of str | ||
""" | ||
|
||
@property | ||
def as_invocation_environment_dict(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -141,10 +141,12 @@ class GCCInstallLocationForLLVM(datatype(['toolchain_dir'])): | |
"""This class is convertible into a list of command line arguments for clang and clang++. | ||
|
||
This is only used on Linux. The option --gcc-toolchain stops clang from searching for another gcc | ||
on the host system. The option appears to only exist on Linux clang and clang++.""" | ||
on the host system. The option appears to only exist on Linux clang and clang++. | ||
""" | ||
|
||
@property | ||
def as_clang_argv(self): | ||
# TODO(#6143): describe exactly what this argument does to the clang/clang++ invocation! | ||
return ['--gcc-toolchain={}'.format(self.toolchain_dir)] | ||
|
||
|
||
|
@@ -161,6 +163,7 @@ def select_llvm_c_toolchain(platform, native_toolchain): | |
llvm_c_compiler_args = [ | ||
'-x', 'c', '-std=c11', | ||
'-nobuiltininc', | ||
'-nostdinc', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've added this in four places and it seems related to the failing unit tests:
As well as in the TestProjectsIntegrationTest.test_shard_44 integration test which shows:
And CTypesIntegrationTest.test_ctypes_third_party_integration which shows:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
] | ||
|
||
if platform.normalized_os_name == 'darwin': | ||
|
@@ -182,7 +185,7 @@ def select_llvm_c_toolchain(platform, native_toolchain): | |
llvm_linker_wrapper = yield Get(LLVMLinker, NativeToolchain, native_toolchain) | ||
llvm_linker = llvm_linker_wrapper.linker | ||
|
||
# TODO: create an algebra so these changes are more clear! | ||
# TODO(#6855): introduce a more concise way to express these compositions of executables. | ||
working_linker = llvm_linker.copy( | ||
path_entries=(llvm_linker.path_entries + working_c_compiler.path_entries), | ||
exe_filename=working_c_compiler.exe_filename, | ||
|
@@ -203,6 +206,7 @@ def select_llvm_cpp_toolchain(platform, native_toolchain): | |
# implementation, or any from the host system. Instead, we use include dirs from the | ||
# XCodeCLITools or GCC. | ||
'-nobuiltininc', | ||
'-nostdinc', | ||
'-nostdinc++', | ||
] | ||
|
||
|
@@ -226,7 +230,7 @@ def select_llvm_cpp_toolchain(platform, native_toolchain): | |
# NB: we use g++'s headers on Linux, and therefore their C++ standard library. | ||
include_dirs=provided_gpp.include_dirs, | ||
extra_args=(llvm_cpp_compiler_args + provided_clangpp.extra_args + gcc_install.as_clang_argv)) | ||
# TODO: why are these necessary? this is very mysterious. | ||
# TODO(#6855): why are these necessary? this is very mysterious. | ||
extra_linking_library_dirs = provided_gpp.library_dirs + provided_clangpp.library_dirs | ||
# Ensure we use libstdc++, provided by g++, during the linking stage. | ||
linker_extra_args=['-stdlib=libstdc++'] | ||
|
@@ -253,12 +257,15 @@ def select_gcc_c_toolchain(platform, native_toolchain): | |
# GCC needs an assembler, so we provide that (platform-specific) tool here. | ||
assembler = yield Get(Assembler, NativeToolchain, native_toolchain) | ||
|
||
gcc_c_compiler_args = [ | ||
'-x', 'c', '-std=c11', | ||
'-nostdinc', | ||
] | ||
|
||
if platform.normalized_os_name == 'darwin': | ||
# GCC needs access to some headers that are only provided by the XCode toolchain | ||
# currently (e.g. "_stdio.h"). These headers are unlikely to change across versions, so this is | ||
# probably safe. | ||
# TODO: we should be providing all of these (so we can eventually phase out XCodeCLITools | ||
# entirely). | ||
xcode_clang = yield Get(CCompiler, XCodeCLITools, native_toolchain._xcode_cli_tools) | ||
new_include_dirs = provided_gcc.include_dirs + xcode_clang.include_dirs | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this swap necessary? (it might have implications for internal use cases, will verify) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually a correction which is necessary for tests to pass (this is due to the gcc headers' use of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...and so this probably deserves a comment in the code. Your cohort which is probably the closest to this code found it mysterious - excellent indicator that it truly is and should get note not just in review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. It has historically been difficult to justify spending time on fixing tech debt for this backend as I wasn't able to get a lot of help understanding the time cost of different approaches, so I attempted to muddle through solutions and leave millions of TODOs because I was extremely concerned about wasting time on unnecessary abstractions. @CMLivingston has been extremely helpful in developing the larger picture. The first longstanding issue is this PR, which addresses the lack of toolchain choice, which you have mentioned multiple times previously (which was appreciated). The second is #6855, which I have listed in a couple comments in this file, and which is intended to remove the mystery from repeated but slightly varying |
||
else: | ||
|
@@ -267,7 +274,7 @@ def select_gcc_c_toolchain(platform, native_toolchain): | |
working_c_compiler = provided_gcc.copy( | ||
path_entries=(provided_gcc.path_entries + assembler.path_entries), | ||
include_dirs=new_include_dirs, | ||
extra_args=['-x', 'c', '-std=c11']) | ||
extra_args=gcc_c_compiler_args) | ||
|
||
gcc_linker_wrapper = yield Get(GCCLinker, NativeToolchain, native_toolchain) | ||
gcc_linker = gcc_linker_wrapper.linker | ||
|
@@ -290,6 +297,7 @@ def select_gcc_cpp_toolchain(platform, native_toolchain): | |
|
||
gcc_cpp_compiler_args = [ | ||
'-x', 'c++', '-std=c++11', | ||
'-nostdinc', | ||
'-nostdinc++', | ||
] | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,6 +77,7 @@ def _assert_ctypes_binary(self, toolchain_variant): | |
|
||
# Check that we have selected the appropriate compilers for our selected toolchain variant, | ||
# for both C and C++ compilation. | ||
# TODO(#6866): don't parse info logs for testing! | ||
for compiler_name in self._compiler_names_for_variant[toolchain_variant]: | ||
self.assertIn("selected compiler exe name: '{}'".format(compiler_name), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am definitely not a fan of scraping logs to implement tests. This was cheap since there was already an integration test but expensive since it adds production logging of questionable value (it effectively parrots the very well tested option system) and that will surprisingly break tests when altered. Consider ~never doing this when possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will make an effort to remember your comment as the concerns you mention are quite bothersome. I would love to implement any alternative -- the info logs here were pretty much exactly for testing purposes and that conflation makes me sick at heart. I suppose the point of an integration test is not to try to inspect internal state but to examine some properties of the result of a pants run. I was wondering whether it was possible to tag an object file or shared library with metadata that includes e.g. which compiler produced it. The rationale for testing both compilers here is to avoid introducing a breakage in one toolchain, and just changing the option value doesn't seem like it does enough to verify that we are choosing the right compiler. It's probably possible to propagate this metadata (which compiler/linker was used) into the produced Internally we have had to address the situation several times where e.g. tensorflow and related wheels assume quite a lot of things about the native code execution environment (mostly relating to glibc and stdlibc++ version), so if there is some way to tag wheels with this type of more in-depth native code compatibility (e.g. which compiler is used), we could potentially apply that method to 3rdparty wheels as well in the future. Bazel might have an analogue of a far more general form of compatibility with e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've linked the above to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would certainly be preferable. That said, just keep scope in mind. All this to test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, what's the status of this? The test still log scrapes. Do you want to follow-up with an issue tracking fixing this test and removing the production log.info it requires? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a link to #6866 to cover my interpretation of this discussion! |
||
pants_run.stdout_data) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is both untrue and doesn't stand the novelty test. Using AbstractClass is a convenient way to properly support
@abstractproperty
but not the only way, and we do this in many places in the codebase, so the revelation - though personally novel is not so generally and so probably not worth a comment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a note to myself which I was planning to remove, and this was a note that I was going to word as "inheriting from AbstractClass, or an equivalent method to set the metaclass", then decided I probably didn't need to go that in depth for now. I am aware this is being presented as code for review, and I agree that this shouldn't be in there, but as mentioned in the other comment, since I "need" travis to run tests, this all gets flattened.
I suppose in the future I could keep TODOs in another file, but that's not ideal and I would prefer to make the local testing situation better to avoid pushing code to a PR that I intend to remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This note has been removed.