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

Fix test_checkstyle.py interpreter constraint #6983

Conversation

Eric-Arellano
Copy link
Contributor

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

Problem

Right now, test_checkstyle.py relies on the assumption that we only allow CPython>=2.7,3.

The tests stop behaving as intended when allowing CPython>=3.6<4, as we will be doing in #6959. This is because the tests are testing how we handle a whitelist of CPython>=3.6, but when we allow this by default, the whitelist no longer matters because the linter will already be working with that interpreter.

Solution

Constrain the test targets to CPython>=3.4,<=3.5, as we no longer support these Python 3 versions, yet they are still meaningful versions. When the whitelist is turned on, the tests will run the linter using either Python 3.4 or 3.5.

Note that constraining to CPython>=3.8 does not work, as Python 3.8 is not yet available so the tests white listing that interpreter will fail due to not finding a corresponding interpreter.

Other solution attempted

I first tried to constrain PythonSetup.global_instance().interpreter_constraints to only allow Python 2.7, as we do now.

I got the constraint to work, but the tests were still failing. I found that this is due to the function PythonSetup.compatibility_or_constraints, which no matter what uses the passed compatibility constraints when given, rather than first trying to read the target's compatibility. So, the Python 3 targets were improperly resolving as Python 2 compatible.

@Eric-Arellano
Copy link
Contributor Author

Let me know if I should be adding @skip_unless_python34_or_python35. I didn't do so already because we don't have this function (nor functionality to allow either interpreter), and it adds a lot of code to have a decorator on every function in test_checkstyle.py (unless there's a way to add it to every test function?).

Thank you!

@Eric-Arellano
Copy link
Contributor Author

CI is failing because PythonInterpreterCache cannot find a 3.4 or 3.5 interpreter:

"No valid interpreters found for targets: [PythonLibrary(a/python:fail3)]\n(filters: (u'CPython>=3.4,<=3.5',))"

I can't figure out why this is and how to fix it. Ubuntu Xenial comes by default with Python 3.5, and we were using this in CI for a week, so it's definitely there and discoverable. Looking at the PythonInterpreterCache, I'm thinking it's looking at what's already in the cache and failing to search for additional interpreter:

# this is from PythonInterpreterCache.setup()`
interpreters.extend(self._setup_cached(filters=filters))
      if not interpreters or unsatisfied_filters():
        interpreters.extend(self._setup_paths(setup_paths, filters=filters))

@jsirois
Copy link
Contributor

jsirois commented Dec 23, 2018

Try <3.6 since 3.5.1 is not <=3.5 (for example).

Thanks for the suggestion John! I didn't realize that's how it resolves.
@Eric-Arellano Eric-Arellano merged commit 29e139f into pantsbuild:master Dec 23, 2018
@Eric-Arellano Eric-Arellano deleted the fix-test-checkstyle-interpreter-constraint branch December 23, 2018 22:20
Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Dec 23, 2018
This reverts commit 1134af5. Instead this problem is being addressed in pantsbuild#6983.
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.

2 participants