Skip to content
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

Exclude Antlr test from testprojects to avoid interpreter conflict #6944

Merged
merged 3 commits into from Dec 17, 2018

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Dec 16, 2018

Problem

Antlr 3 only works with Python 2, so we have to constrain that part to only using Python 2, as done in #6924.

However, this results in one of the testprojects shards failing when using Python 3 due to mixing a Py2-only constraint with a Py3-only constraint.

$ export PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS='["CPython>=3.5,<4"]'
$ ./pants clean-all test testprojects/src/python/antlr:eval testprojects/src/python/antlr:eval-bin testprojects/src/python/coordinated_runs:creator testprojects/src/python/coordinated_runs:phaser testprojects/src/python/coordinated_runs:waiter testprojects/src/python/print_env:print_env

...

18:47:02 00:04   [pyprep]
18:47:02 00:04     [interpreter]
                   Invalidated 4 targets.
FAILURE: Unable to detect a suitable interpreter for compatibilities: CPython>=2.7,<3 && CPython>=3.5,<4 (Conflicting targets: testprojects/src/python/antlr:eval, testprojects/src/python/coordinated_runs:creator)

We can get Antlr to evaluate correctly when there are no other targets, but test_testprojects_integration.py works by running multiple targets with one single command so this does not help.

$ export PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS='["CPython>=3.5,<4"]'
$ ./pants clean-all test testprojects/src/python/antlr:eval testprojects/src/python/antlr:eval-bin 

{interpreter will resolve correctly}

Solution

Exclude the Antlr testproject from projects/testprojects. The code is already covered by the tests in backend/codegen/antlr/test_antlr_py_gen_integration.py.

This is a much simpler solution than trying to come up with logic in test_testprojects_integration.py to handle interpreter conflicts like this, e.g. putting Python 2 only targets in one dedicated shard.

Also fixes duplicate .so binary names

By removing the antlr test, the shards were redistributed in test_testprojects_integration.py. This led to python_distribution/ctypes and python_distribution/ctypes_with_extra_compiler_flags running at the same time, which resulted in the error FAILURE: The name 'asdf-cpp' was used for two shared libraries.

We fix this by making the asdf-cpp name unique for all of the 3 uses.

@Eric-Arellano
Copy link
Contributor Author

Question for reviewers to make sure we aren't losing any test coverage.

test_antlr_py_gen_integration.py runs these two commands:

$ ./pants run testprojects/src/python/antlr:eval-bin --run-py-args="123 * 321"
$ ./pants gen testprojects/src/antlr/python/test:antlr_failure

This is compared to test_testprojects_integration.py running

./pants --no-pantsrc --pants-workdir=./.pants.d/tmp/tmpkgjg0twx.pants.d --kill-nailguns --print-exception-stacktrace=True test testprojects/src/python/antlr:eval testprojects/src/python/antlr:eval-bin --jvm-platform-default-platform=java7 --gen-protoc-import-from-root

Are ./pants gen and ./pants run comprehensive enough to have the same effect? Or should I add a new integration test to test_antlr_py_gen_integration.py to call ./pants test? The test goal is not included in either of the two task graphs.

Copy link
Member

@jsirois jsirois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is 0 coverage lost.

@Eric-Arellano
Copy link
Contributor Author

Eric-Arellano commented Dec 16, 2018

Because this exclusion changed what each shard has, shard 43 is now failing from this command:

./pants test testprojects/src/python/python_distribution/ctypes:ctypes testprojects/src/python/python_distribution/ctypes_interop/some-math:some-math testprojects/src/python/python_distribution/ctypes_interop/some-more-math:some-more-math testprojects/src/python/python_distribution/ctypes_interop/wrapped-math:wrapped-math testprojects/src/python/python_distribution/ctypes_interop:bin testprojects/src/python/python_distribution/ctypes_interop:ctypes_interop testprojects/src/python/python_distribution/ctypes_with_extra_compiler_flags:cpp_library --jvm-platform-default-platform=java7 --gen-protoc-import-from-root

...

FAILURE: The name 'asdf-cpp' was used for two shared libraries: SharedLibrary(name=asdf-cpp, path=./pants/.pants.d/link/shared-libraries/75beb223f481/testprojects.src.python.python_distribution.ctypes.cpp_library/current/libasdf-cpp.dylib) and SharedLibrary(name=asdf-cpp, path=./pants/.pants.d/link/shared-libraries/75beb223f481/testprojects.src.python.python_distribution.ctypes_with_extra_compiler_flags.cpp_library/current/libasdf-cpp.dylib).

I think this is expected behavior, right? Ideas on how to not have the two tests run in the same shard? Is the only solution to exclude one of them?

Edit: e6137eb should fix this.

Fixes the error `FAILURE: The name 'asdf-cpp' was used for two shared libraries` when python_distribution/ctypes and python_distribution/ctypes_with_extra_compiler_flags are run at the same time.
Copy link
Contributor

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@Eric-Arellano Eric-Arellano merged commit dbc7ef9 into pantsbuild:master Dec 17, 2018
@Eric-Arellano Eric-Arellano deleted the skip-antlr-test branch December 17, 2018 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants