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

Introduce libc subsystem to find crti.o on linux hosts and unskip the native backend subsystem tests #5943

Merged
merged 74 commits into from Jul 2, 2018

Conversation

Projects
None yet
4 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Jun 8, 2018

Problem

The native backend subsystems tests introduced in #5490 are still skipped, complaining about crti.o on linux, which is part of libc. See #5662 -- clang's driver will find the directory containing that file on travis, but gcc won't. We should make a way to find this file (which is necessary for creating executables) so we can unskip the native backend testing.

Solution

  • Fix a mistake in #5780 -- we did not check the correct directory with os.path.isdir(), so we never found the LLVM BinaryTool when downloading it from the LLVM download page.
  • Find libc using the new LibcDev subsystem. This uses the option --libc-dir, if provided, or finds an installation of libc with crti.o by invoking --host-compiler on the host once to get its search directories (this is necessary on travis, due to ubuntu's nonstandard installation location).
  • Expand the definition of executables, compilers, and linkers in src/python/pants/backend/native/config/environment.py to cover everything needed to actually compile, and give them the ability to generate an environment suitable for passing into subprocess.Popen().
  • Introduce GCCCCompiler, GCCCppCompiler, LLVMCCompiler, and LLVMCppCompiler to differentiate between the two different compilers we have available for each language.
  • Expose the libc lib directory to the compilers we create in native_toolchain.py.
  • Unskip tests in test_native_toolchain.py from #5490.
  • Make the default CCompiler and CppCompiler into clang, for no particular reason (it will pass CI with gcc as well).

The different compilers we can use will likely be denoted using variants in the future, but this solution allows us to separate the resources generated from each subsystem (GCC, LLVM, Binutils, XCodeCLITools) from a fully-functioning compiler that can be invoked (because actually invoking either clang or gcc requires some resources from the other, e.g. headers and libraries). Now, each binary tool only does the job of wrapping the resources it contains, while native_toolchain.py does the job of creating either a clang or a gcc compiler that we can invoke on the current host (with all necessary headers, libs, etc).

Result

The native toolchain tests from #5490 can be unskipped, and we can invoke our toolchain on almost any linux installation without further setup. The tests in test_native_toolchain.py now pass in CI, and as we iterate on the native backend, we will continue to have end-to-end testing for both of our compilers, on osx as well, regardless of whichever one we choose to use for python_dist() compilation.

@cosmicexplorer cosmicexplorer referenced this pull request Jun 8, 2018

Closed

Add glibc binary #70

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jun 8, 2018

This would completely resolve one of the FIXMEs currently left in #5815, which includes /usr/bin in the PATH when compiling native code in python_dist() sources (not through ctypes), if it works. As mentioned in pantsbuild/binaries#70, I can't figure out to repro the issue happening on Travis, even when I try to invoke pants from the docker images in this repo. If there are some canonical instructions for how to do that, it would be helpful in testing this PR.


@memoized_method
def glibc_dir(self):
# FIXME: this can be removed once #5815 lands (GCC should be made to depend on GLibc on Linux).

This comment has been minimized.

@benjyw

benjyw Jun 10, 2018

Contributor

It's not clear what the "this" is that can be removed.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 10, 2018

Contributor

Made it a lot more specific in c3aa525, thanks!

@benjyw

benjyw approved these changes Jun 10, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Works for me :) Thanks!

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:introduce-glibc-for-crti.o branch from bce6634 to 14b8a7d Jun 13, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jun 14, 2018

Had a good discussion with @kwlzn on the slack today, and I now believe this should be replaced with a method of searching for crti.o and other files that are necessarily host-specific, very much like what we're doing with XCodeCLITools. I will refactor this PR and comment again when it is finished.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jun 14, 2018

It looks like we'll have to invoke the system gcc -print-search-dirs, then parse the output of that to find candidates for dirs containing crti.o (I already have code to do this, this interface is very stable). Since we're not using the system gcc to compile anything (so that we can e.g. support newer c++ features), we have to do this bootstrap step in order to create executables and to compile python_dist()s, because the location of the host-specific crti.o is partially baked into the gcc installation (as in, contains stuff that the distro gcc package maintainer added). We'll also need to add the libc6-dev package to the trusty images we use in CI (otherwise, they don't have crti.o). The native backend subsystem tests will ensure we can use clang to compile c/c++ as well using this -- the only reason we need the system gcc is to get the location of crti.o.

The above is all only for Linux hosts. We'll have a good error message like XCodeCLITools if crti.o could not be found. Implementing this now.

@cosmicexplorer cosmicexplorer changed the title Introduce glibc BinaryTool on linux to source crti.o and unskip the native backend subsystem tests Introduce libc subsystem to find crti.o on linux hosts and unskip the native backend subsystem tests Jun 15, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jun 15, 2018

This PR should be ready for review again. Instead of providing glibc, we are finding the host's libc installation by parsing the output of gcc -print-search-dirs from the host gcc (this is done in a configurable way, so not necessarily with the host's gcc using --host-compiler, or at all if the libc location is known in advance, by providing --libc-dir). We don't just need libc, we need what some distros label libc-dev or libc6-dev, which provides crti.o, which is necessary to e.g. create executables or compile python_dist()s. This lets us pass the now-unskipped native backend subsystem tests.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jun 15, 2018

According to the CI failures on the unit tests, we need to expose our provided gcc's libstdc++.so.6 to clang through LIBRARY_PATH for it to work if we set LIBRARY_PATH to use the host libc installation. I'll work on doing that now.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jun 15, 2018

will wait to review.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:introduce-glibc-for-crti.o branch 2 times, most recently from 2c23923 to 1335922 Jun 23, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jun 28, 2018

I have updated the OP and CI is green. Please take another look -- this was extremely difficult and time-intensive to get working (mostly because it's very difficult to reproduce the travis environment, and yes I really did try), but after this I am confident we will be able to iterate on the native backend (and the way we invoke compilers, linkers, etc) without fear, which seems like a useful result. The tools we have developed here are not specific to travis whatsoever and are generic enough to make a reliable basis for further extension.

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

def register_options(cls, register):
super(LibcDev, cls).register_options(register)

register('--libc-dir', type=dir_option, default='/usr/lib', advanced=True,

This comment has been minimized.

@stuhood

stuhood Jun 28, 2018

Member

Should both of these options be fingerprinted?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 28, 2018

Contributor

Yes!! Will fix and keep that in mind in the future.

This comment has been minimized.

@cosmicexplorer
@@ -26,75 +24,160 @@ class XCodeCLITools(Subsystem):

options_scope = 'xcode-cli-tools'

_REQUIRED_TOOLS = frozenset(['cc', 'c++', 'clang', 'clang++', 'ld', 'lipo'])
REQUIRED_FILES = {

This comment has been minimized.

@stuhood

stuhood Jun 28, 2018

Member

Are these intentionally public? Would be good to encapsulate instead if possible, to avoid needing to break APIs to add more data.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 28, 2018

Contributor

Yeah, there's no reason for these to be public and the reason you mentioned is a fantastic reason to keep it private. Will fix.

register('--xcode-cli-install-location', type=dir_option, default='/usr/bin', advanced=True,
help='Installation location for the XCode command-line developer tools.')
register('--install-prefixes', type=list, default=cls.INSTALL_PREFIXES_DEFAULT, advanced=True,
help='Locations to search for resources from the XCode CLI tools, including a '

This comment has been minimized.

@stuhood

stuhood Jun 28, 2018

Member

ditto fingerprint.

This comment has been minimized.

@cosmicexplorer
self._scheduler.add_root_selection(native_execution_request, subject, product)
return ExecutionRequest(request_specs, native_execution_request)

# FIXME: note that this does a cross prod!

This comment has been minimized.

@stuhood

stuhood Jun 28, 2018

Member

You updated the comment here... can remove.

This comment has been minimized.

@cosmicexplorer
as "list of product".
An ExecutionRequest for an Address represents exactly one product output, as does
SingleAddress. But we differentiate between them here in order to normalize the output for all
Spec objects as "list of product".

This comment has been minimized.

@stuhood

stuhood Jun 28, 2018

Member

I don't think this is true any longer... can nuke this paragraph probably.

This comment has been minimized.

@cosmicexplorer
@@ -406,15 +406,25 @@ def visualize_graph_to_file(self, filename):
def visualize_rule_graph_to_file(self, filename):
self._scheduler.visualize_rule_graph_to_file(filename)

def execution_request_literal(self, request_specs):

This comment has been minimized.

@stuhood

stuhood Jun 28, 2018

Member

I think that rather than adding a new method, we'd want to just update execute_request... it only has two callers outside of unit tests. But that would increase the scope here... if you're able to swap out the methods as a quick followup (not here, as this is already a bit large), that would be appreciated.

Also, the SchedulerSession.product_request and product_requests family is easier to use... so adding another method that is shaped like "list of tuples" would be ideal (and maybe even removing/deprecating ExecutionRequest as a public API).

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 28, 2018

Contributor

That all sounds like a viable scope for a followup -- I'll make an issue.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 28, 2018

Contributor

Made into #6044!

cosmicexplorer added some commits Jun 8, 2018

cosmicexplorer added some commits Jun 27, 2018

respond to review comments
- add fingerprint=True to new subsystem options
- make _REQUIRED_FILES private
- add a useful comment
- remove a now-false paragraph from the execution_request() docstring

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:introduce-glibc-for-crti.o branch from 6520d79 to 08ba46d Jun 29, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 2, 2018

Feel free to merge when ready.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jul 2, 2018

Travis isn't showing the passing build in this thread for some reason, but 08ba46d has indeed passed CI, so merging now.

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