-
-
Notifications
You must be signed in to change notification settings - Fork 632
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 basic native task unit tests. #6179
Add basic native task unit tests. #6179
Conversation
55a4f4e
to
1b013e2
Compare
NB: When I wrote these tests there were 2 issues, |
This is hitting:
I'll dig a bit on the |
I find:
And afaict, the existing search algorithm only consults this (outside amending the search path manually via flags):
That has none of the right answers. Just @cosmicexplorer I think you've banged your head against this a good deal. Anything obvious you think I'm missing re searching for |
Gah, I was trying really hard to not make this a problem to inflict on anyone else. Indeed #5998 fixed the |
@jsirois I'm not certain how the ld.so cache would help in locating It looks like the output of that command contains |
Ok, I'm 90% sure that we shouldn't be seeing this error with |
### Problem Trying to release `1.9.0rc0` failed, and pypi won't accept another release to the same version. #6192 will have more information about what happened here after we can get this release out. ### Solution Make a noop release for `1.9.0rc1`. ### Result We finally have a release candidate for 1.9.0. I plan to fast-follow with a hotfix release `rc2` containing the changes from both #6201 and #6179, as these both describe bugged functionality that would make it very difficult to use `python_dist()` and the new C/C++ targets.
### Problem Trying to release `1.9.0rc0` failed, and pypi won't accept another release to the same version. #6192 will have more information about what happened here after we can get this release out. ### Solution Make a noop release for `1.9.0rc1`. ### Result We finally have a release candidate for 1.9.0. I plan to fast-follow with a hotfix release `rc2` containing the changes from both #6201 and #6179, as these both describe bugged functionality that would make it very difficult to use `python_dist()` and the new C/C++ targets.
So after doing a little more research, I'm convinced I was mistaken on the purpose of I believe part of the reason for my confusion is that the "init" facility provided by (this information should be cataloged in an issue somewhere, probably, or maybe a README in the native backend. i'll take responsibility for doing that) Regardless of the correct interpretation, it now seems (more) clear that |
This travis run on my fork of this PR, rebased onto #6217, is now magically passing everything, because we fix the linker provided to LinkSharedLibraries. Also, this could resolve #5934 when merged. |
…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.
660c600
to
28c24d9
Compare
@cosmicexplorer please take another look. Now green rebased on top of your fix with 1 added test for the C compilation task. |
Running a `./pants lint` across the whole repo revealed an issue accessing a non-existant `self.linker.platform` attribute in `LinkSharedLibraries`. Add basic unit tests to exercise task execution with a full cache miss and then a full hit.
28c24d9
to
5258fc9
Compare
@cosmicexplorer please take a final look. Re-based on top a conflicting change from @illicitonion |
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.
@cosmicexplorer is under the weather today, but this looks sane to me. Thanks!
Thanks @stuhood this is in master. The cherry-picked to 1.9.x was not clean, so I'll leave that to you @cosmicexplorer to decide whether worth it or not. I suspect not since this turned into just adding tests with the original 2 production issues fixed by subsequent PRs. |
…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.
Running a `./pants lint` across the whole repo revealed an issue accessing a non-existant `self.linker.platform` attribute in `LinkSharedLibraries`. Add basic unit tests to exercise task execution with a full cache miss and then a full hit.
### Problem Trying to release `1.9.0rc0` failed, and pypi won't accept another release to the same version. pantsbuild#6192 will have more information about what happened here after we can get this release out. ### Solution Make a noop release for `1.9.0rc1`. ### Result We finally have a release candidate for 1.9.0. I plan to fast-follow with a hotfix release `rc2` containing the changes from both pantsbuild#6201 and pantsbuild#6179, as these both describe bugged functionality that would make it very difficult to use `python_dist()` and the new C/C++ targets.
…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.
Running a `./pants lint` across the whole repo revealed an issue accessing a non-existant `self.linker.platform` attribute in `LinkSharedLibraries`. Add basic unit tests to exercise task execution with a full cache miss and then a full hit.
Running a
./pants lint
across the whole repo revealed an issueaccessing a non-existant
self.linker.platform
attribute inLinkSharedLibraries
. Add basic unit tests to exercise task executionwith a full cache miss and then a full hit.