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

Fix pydist native sources selection #6205

Merged
merged 17 commits into from Jul 27, 2018

Conversation

Projects
None yet
4 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Jul 20, 2018

Problem

This adds a few more unit tests on top of #6201 -- specifically, testing in isolation that the platform is correctly set on a python_dist() which has no native sources itself, but depends on a ctypes_compatible_c(pp)?_library() (which is what #6201 fixes). It also moves all testing related to the ctypes_compatible_c(pp)?_library() targets into separate testing files to separate functionality more clearly.

Resolves #6201.

@cosmicexplorer cosmicexplorer requested review from CMLivingston , stuhood , jsirois , benjyw and kwlzn and removed request for CMLivingston Jul 20, 2018

[os.path.abspath(obj) for obj in object_files])
self._get_third_party_lib_args(link_request) +
['-o', os.path.join(buildroot, resulting_shared_lib_path)] +
[os.path.join(buildroot, obj) for obj in object_files])

This comment has been minimized.

@CMLivingston

CMLivingston Jul 20, 2018

Contributor

is obj a path to somewhere? or do we have object files in the build root dir and why?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 20, 2018

Contributor

Joining against the buildroot is necessary in compile to get correct paths to source files when we modify the buildroot (e.g. in our ctypes unit testing). I don't think this is correct for object files and other things stored in .pants.d and I will remove that now and I think it will work.

This comment has been minimized.

@CMLivingston

CMLivingston Jul 20, 2018

Contributor

👍

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 20, 2018

Contributor

Great catch, thanks a ton. Fixed in f86aedd!

@@ -28,12 +29,15 @@ class LinkSharedLibraryRequest(datatype([
'object_files',
'native_artifact',
'output_dir',
# This may be None!
'external_libs_info'
])): pass

This comment has been minimized.

@jsirois

jsirois Jul 20, 2018

Member

Consider over-riding __new__ and having it take external_libs_info=None to make this more clear - or maybe provide a factory @classmethod to do said same.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 20, 2018

Contributor

Done (with several other changes along those lines) in 643417e!

@@ -64,7 +64,7 @@ def native_target_has_native_sources(self, target):
def _native_target_matchers(self):
return {
Exactly(PythonDistribution): self.pydist_has_native_sources,

This comment has been minimized.

@jsirois

jsirois Jul 20, 2018

Member

In the current world, all public targets exposed by register.py's can be subclassed for purposes you-know-not making Exactly on these Target types probably never advised. It seems like you'd stll want to operate on a class FoursquarePythonDistribution(PythonDistribution) that might add information used by a Foursquare custom task or just act like a macro, sealing in certain Foursquare specific values.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 20, 2018

Contributor

You're totally right. I will make that change.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 20, 2018

Contributor

Done in ef41174, along with making _PYTHON_PLATFORM_TARGETS_CONSTRAINT = SubclassesOf(PythonBinary, PythonDistribution) (instead of Exactly as well). I assumed that change would be correct for the same reasons.

@@ -31,4 +31,5 @@ python_binary(
dependencies=[
':ctypes',
],
platforms="current"

This comment has been minimized.

@jsirois

jsirois Jul 20, 2018

Member

It may be true platforms accepts string or list of strings, but it's plural and this feature is not documented. Stick to ['current'] for local readability? I had to go check all this before flagging as a bug for example since it's not locally readable.

This comment has been minimized.

@cosmicexplorer

('src/python/plat_specific_c_dist:ctypes_c_library', {
'key': 'ctypes_c_library',
'target_type': CLibrary,

This comment has been minimized.

@CMLivingston

CMLivingston Jul 20, 2018

Contributor

Might be able to remove target_type key? I can't find any usages.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 20, 2018

Contributor

It's in the superclass -- it's one of the kwargs that is forwarded to self.make_target().

This comment has been minimized.

@CMLivingston

CMLivingston Jul 20, 2018

Contributor

👍

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 20, 2018

Contributor

(if that's too indirect let me know if there would be a useful comment to add!)

@@ -0,0 +1,117 @@
# coding=utf-8
# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).

This comment has been minimized.

@CMLivingston

CMLivingston Jul 20, 2018

Contributor

Can bump to 2018

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 20, 2018

Contributor

Good catch. This might be something that a linter could help with in the future?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 20, 2018

Contributor

Fixed in 912e491!

@CMLivingston
Copy link
Contributor

CMLivingston left a comment

This looks great, thanks for taking the time to refactor tests. Just a couple comments from me w/ no blockers, however I agree with John that we should subclass py_dist as well.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jul 21, 2018

These changes are (predictably?) producing the same error as in #6179:

Starting workunit link-shared-libraries
                     /home/travis/.cache/pants/bin/binutils/linux/x86_64/2.30/binutils/bin/ld: cannot find crti.o: No such file or directory
                     /home/travis/.cache/pants/bin/binutils/linux/x86_64/2.30/binutils/bin/ld: cannot find -lm

Time to knock out one bird with two stones.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jul 24, 2018

This travis run of this PR rebased onto #6217 magically passes, because the linker "just works" now.

cosmicexplorer added a commit that referenced this pull request Jul 24, 2018

Associate cli arguments with executables and refactor llvm/gcc c/c++ …
…toolchain selection (#6217)

### Problem

#5951 explains the problem addressed by moving CLI arguments to individual `Executable` objects -- this reduces greatly the difficulty in generating appropriate command lines for the executables invoked. In this PR, it can be seen to remove a significant amount of repeated boilerplate.

Additionally, we weren't distinguishing between a `Linker` to link the compiled object files of `gcc` or `g++` vs `clang` or `clang++`. We were attempting to generate a linker object which would work with *any of* `gcc`, `g++`, `clang`, or `clang++`, and this wasn't really feasible. Along with the above, this made it extremely difficult and error-prone to generate correct command lines / environments for executing the linker, which led to e.g. not being able to find `crti.o` (as one symptom addressed by this problem).

### Solution

- Introduce `CToolchain` and `CppToolchain` in `environment.py`, which can be generated from `LLVMCToolchain`, `LLVMCppToolchain`, `GCCCToolchain`, or `GCCCppToolchain`. These toolchain datatypes are created in `native_toolchain.py`, where a single `@rule` for each ensures that no C or C++ compiler that someone can request was made without an accompanying linker, which will be configured to work with the compiler.
- Introduce the `extra_args` property to the `Executable` mixin in `environment.py`, which `Executable` subclasses can just declare a datatype field named `extra_args` in order to override. This is used in `native_toolchain.py` to ensure platform-specific arguments and environment variables are set in the same `@rule` which produces a paired compiler and linker -- there is a single place to look at to see where all the process invocation environment variables and command-line arguments are set for a given toolchain.
- Introduce the `ArchiveFileMapper` subsystem and use it to declare sets of directories to resolve within our BinaryTool archives `GCC` and `LLVM`. This subsystem allows globbing (and checks that there is a unique expansion), which makes it robust to e.g. platform-specific paths to things like include or lib directories.

### Result

Removes several FIXMEs, including heavily-commented parts of `test_native_toolchain.py`. Partially addresses #5951 -- `setup_py.py` still generates its own execution environment from scratch, and this could be made more hygienic in the future. As noted in #6179 and #6205, this PR seems to immediately fix the CI failures in those PRs.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:fix-pydist-has-native-sources branch from 912e491 to 34202b2 Jul 24, 2018

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jul 26, 2018

Re-started the rust test shard since it failed on the known-flaky successful_execution_after_four_getoperations.

Chris Livingston and others added some commits Jul 19, 2018

cosmicexplorer added some commits Jul 20, 2018

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:fix-pydist-has-native-sources branch from 74c193d to 663e028 Jul 27, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jul 27, 2018

Rebased, should pass CI.

@cosmicexplorer cosmicexplorer added this to the 1.9.x milestone Jul 27, 2018

@cosmicexplorer cosmicexplorer merged commit c2ca377 into pantsbuild:master Jul 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

cosmicexplorer added a commit that referenced this pull request Jul 28, 2018

Fix pydist native sources selection (#6205)
This adds a few more unit tests on top of #6201 -- specifically, testing in isolation that the platform is correctly set on a `python_dist()` which has no native sources itself, but depends on a `ctypes_compatible_c(pp)?_library()` (which is what #6201 fixes). It also moves all testing related to the `ctypes_compatible_c(pp)?_library()` targets into separate testing files to separate functionality more clearly.

Resolves #6201.

cosmicexplorer added a commit that referenced this pull request Jul 28, 2018

Associate cli arguments with executables and refactor llvm/gcc c/c++ …
…toolchain selection (#6217)

### Problem

#5951 explains the problem addressed by moving CLI arguments to individual `Executable` objects -- this reduces greatly the difficulty in generating appropriate command lines for the executables invoked. In this PR, it can be seen to remove a significant amount of repeated boilerplate.

Additionally, we weren't distinguishing between a `Linker` to link the compiled object files of `gcc` or `g++` vs `clang` or `clang++`. We were attempting to generate a linker object which would work with *any of* `gcc`, `g++`, `clang`, or `clang++`, and this wasn't really feasible. Along with the above, this made it extremely difficult and error-prone to generate correct command lines / environments for executing the linker, which led to e.g. not being able to find `crti.o` (as one symptom addressed by this problem).

### Solution

- Introduce `CToolchain` and `CppToolchain` in `environment.py`, which can be generated from `LLVMCToolchain`, `LLVMCppToolchain`, `GCCCToolchain`, or `GCCCppToolchain`. These toolchain datatypes are created in `native_toolchain.py`, where a single `@rule` for each ensures that no C or C++ compiler that someone can request was made without an accompanying linker, which will be configured to work with the compiler.
- Introduce the `extra_args` property to the `Executable` mixin in `environment.py`, which `Executable` subclasses can just declare a datatype field named `extra_args` in order to override. This is used in `native_toolchain.py` to ensure platform-specific arguments and environment variables are set in the same `@rule` which produces a paired compiler and linker -- there is a single place to look at to see where all the process invocation environment variables and command-line arguments are set for a given toolchain.
- Introduce the `ArchiveFileMapper` subsystem and use it to declare sets of directories to resolve within our BinaryTool archives `GCC` and `LLVM`. This subsystem allows globbing (and checks that there is a unique expansion), which makes it robust to e.g. platform-specific paths to things like include or lib directories.

### Result

Removes several FIXMEs, including heavily-commented parts of `test_native_toolchain.py`. Partially addresses #5951 -- `setup_py.py` still generates its own execution environment from scratch, and this could be made more hygienic in the future. As noted in #6179 and #6205, this PR seems to immediately fix the CI failures in those PRs.

cosmicexplorer added a commit that referenced this pull request Jul 28, 2018

Fix pydist native sources selection (#6205)
### Problem

This adds a few more unit tests on top of #6201 -- specifically, testing in isolation that the platform is correctly set on a `python_dist()` which has no native sources itself, but depends on a `ctypes_compatible_c(pp)?_library()` (which is what #6201 fixes). It also moves all testing related to the `ctypes_compatible_c(pp)?_library()` targets into separate testing files to separate functionality more clearly.

Resolves #6201.

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

Associate cli arguments with executables and refactor llvm/gcc c/c++ …
…toolchain selection (pantsbuild#6217)

### Problem

pantsbuild#5951 explains the problem addressed by moving CLI arguments to individual `Executable` objects -- this reduces greatly the difficulty in generating appropriate command lines for the executables invoked. In this PR, it can be seen to remove a significant amount of repeated boilerplate.

Additionally, we weren't distinguishing between a `Linker` to link the compiled object files of `gcc` or `g++` vs `clang` or `clang++`. We were attempting to generate a linker object which would work with *any of* `gcc`, `g++`, `clang`, or `clang++`, and this wasn't really feasible. Along with the above, this made it extremely difficult and error-prone to generate correct command lines / environments for executing the linker, which led to e.g. not being able to find `crti.o` (as one symptom addressed by this problem).

### Solution

- Introduce `CToolchain` and `CppToolchain` in `environment.py`, which can be generated from `LLVMCToolchain`, `LLVMCppToolchain`, `GCCCToolchain`, or `GCCCppToolchain`. These toolchain datatypes are created in `native_toolchain.py`, where a single `@rule` for each ensures that no C or C++ compiler that someone can request was made without an accompanying linker, which will be configured to work with the compiler.
- Introduce the `extra_args` property to the `Executable` mixin in `environment.py`, which `Executable` subclasses can just declare a datatype field named `extra_args` in order to override. This is used in `native_toolchain.py` to ensure platform-specific arguments and environment variables are set in the same `@rule` which produces a paired compiler and linker -- there is a single place to look at to see where all the process invocation environment variables and command-line arguments are set for a given toolchain.
- Introduce the `ArchiveFileMapper` subsystem and use it to declare sets of directories to resolve within our BinaryTool archives `GCC` and `LLVM`. This subsystem allows globbing (and checks that there is a unique expansion), which makes it robust to e.g. platform-specific paths to things like include or lib directories.

### Result

Removes several FIXMEs, including heavily-commented parts of `test_native_toolchain.py`. Partially addresses pantsbuild#5951 -- `setup_py.py` still generates its own execution environment from scratch, and this could be made more hygienic in the future. As noted in pantsbuild#6179 and pantsbuild#6205, this PR seems to immediately fix the CI failures in those PRs.

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

Fix pydist native sources selection (pantsbuild#6205)
### Problem

This adds a few more unit tests on top of pantsbuild#6201 -- specifically, testing in isolation that the platform is correctly set on a `python_dist()` which has no native sources itself, but depends on a `ctypes_compatible_c(pp)?_library()` (which is what pantsbuild#6201 fixes). It also moves all testing related to the `ctypes_compatible_c(pp)?_library()` targets into separate testing files to separate functionality more clearly.

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