Fix pydist native sources selection #6205

cosmicexplorer · 2018-07-20T16:56:19Z

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 · 2018-07-20T17:18:00Z

src/python/pants/backend/native/tasks/link_shared_libraries.py

-           [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])


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

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.

Great catch, thanks a ton. Fixed in f86aedd!

jsirois · 2018-07-20T17:21:23Z

src/python/pants/backend/native/tasks/link_shared_libraries.py

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


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.

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

jsirois · 2018-07-20T17:26:00Z

src/python/pants/backend/python/subsystems/python_native_code.py

@@ -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,


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.

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

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.

jsirois · 2018-07-20T17:29:16Z

testprojects/src/python/python_distribution/ctypes/BUILD

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


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.

Fixed in e64614f! cc @CMLivingston

CMLivingston · 2018-07-20T17:35:38Z

tests/python/pants_test/backend/python/tasks/test_ctypes.py

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


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

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

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

CMLivingston · 2018-07-20T17:36:09Z

tests/python/pants_test/backend/python/tasks/test_ctypes.py

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


Can bump to 2018

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

Fixed in 912e491!

CMLivingston

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 · 2018-07-21T01:15:04Z

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 · 2018-07-24T07:36:04Z

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

…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.

jsirois · 2018-07-26T20:00:14Z

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

… for ctypes_* targets

…ream

…d downstream" This reverts commit 6e5565e.

cosmicexplorer · 2018-07-27T20:58:10Z

Rebased, should pass CI.

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.

…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.

### 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.

…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.

### 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.

cosmicexplorer requested review from CMLivingston, stuhood, jsirois, benjyw and kwlzn and removed request for CMLivingston July 20, 2018 16:56

cosmicexplorer added the needs-cherrypick label Jul 20, 2018

CMLivingston reviewed Jul 20, 2018

View reviewed changes

jsirois approved these changes Jul 20, 2018

View reviewed changes

CMLivingston reviewed Jul 20, 2018

View reviewed changes

CMLivingston approved these changes Jul 20, 2018

View reviewed changes

jsirois approved these changes Jul 20, 2018

View reviewed changes

This was referenced Jul 22, 2018

Add basic native task unit tests. #6179

Merged

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

Merged

cosmicexplorer force-pushed the fix-pydist-has-native-sources branch from 912e491 to 34202b2 Compare July 24, 2018 23:17

Chris Livingston and others added 9 commits July 27, 2018 13:43

Use subclassesOf instead of Exactly to enable native source detection…

cff11b8

… for ctypes_* targets

First round of iteration

18af444

add testing for the platform fix

14ec2ef

split off python dist generation test base into a util subdir

909fc61

remove unnecessary BUILD file deps

94070bd

fix up platform selections for tests

2756fc6

add very helpful commenting

7d90de4

remove incorrect path join against buildroot

325a3f0

decouple external libs product from the fetch task

46957bd

cosmicexplorer added 8 commits July 27, 2018 13:47

convert Exactly -> SubclassesOf as per review comments

5eb07d6

keep platforms= plural for now

c846f84

fix copyright year

bad9bb3

add missing Linker import

e6b4d76

remove now-unused Exactly import

de6a7a6

revert changes to PythonDistribution due to failure identified downst…

bcdfe90

…ream

remove nonexistent python backend rules

cd9b45d

Revert "revert changes to PythonDistribution due to failure identifie…

663e028

…d downstream" This reverts commit 6e5565e.

cosmicexplorer force-pushed the fix-pydist-has-native-sources branch from 74c193d to 663e028 Compare July 27, 2018 20:48

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

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

stuhood removed the needs-cherrypick label Aug 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pydist native sources selection #6205

Fix pydist native sources selection #6205

cosmicexplorer commented Jul 20, 2018

CMLivingston Jul 20, 2018

cosmicexplorer Jul 20, 2018

CMLivingston Jul 20, 2018

cosmicexplorer Jul 20, 2018

jsirois Jul 20, 2018

cosmicexplorer Jul 20, 2018

jsirois Jul 20, 2018 •

edited

Loading

cosmicexplorer Jul 20, 2018

cosmicexplorer Jul 20, 2018

jsirois Jul 20, 2018 •

edited

Loading

cosmicexplorer Jul 20, 2018

CMLivingston Jul 20, 2018

cosmicexplorer Jul 20, 2018

CMLivingston Jul 20, 2018

cosmicexplorer Jul 20, 2018

CMLivingston Jul 20, 2018

cosmicexplorer Jul 20, 2018

cosmicexplorer Jul 20, 2018

CMLivingston left a comment

cosmicexplorer commented Jul 21, 2018

cosmicexplorer commented Jul 24, 2018 •

edited

Loading

jsirois commented Jul 26, 2018

cosmicexplorer commented Jul 27, 2018 •

edited

Loading

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

Fix pydist native sources selection #6205

Fix pydist native sources selection #6205

Conversation

cosmicexplorer commented Jul 20, 2018

Problem

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsirois Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsirois Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CMLivingston left a comment

Choose a reason for hiding this comment

cosmicexplorer commented Jul 21, 2018

cosmicexplorer commented Jul 24, 2018 • edited Loading

jsirois commented Jul 26, 2018

cosmicexplorer commented Jul 27, 2018 • edited Loading

jsirois Jul 20, 2018 •

edited

Loading

jsirois Jul 20, 2018 •

edited

Loading

cosmicexplorer commented Jul 24, 2018 •

edited

Loading

cosmicexplorer commented Jul 27, 2018 •

edited

Loading