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.

Sign up for GitHub

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

Jump to bottom

Fix CI failures introduced by #6275 #6454

Merged
merged 12 commits into from
Sep 5, 2018
Merged

Fix CI failures introduced by #6275 #6454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
cec28fb
rstrip -> strip to pass the py3 unit test shard
cosmicexplorer Sep 5, 2018
1f6ac03
add PEX_VERBOSE=9 to a test failing with a resolution error
cosmicexplorer Sep 5, 2018
93516af
reduce test shards to the failing ones
cosmicexplorer Sep 5, 2018
5e63777
revert incorrect fix
cosmicexplorer Sep 5, 2018
a4862cf
plumb PEX_VERBOSE into executing pants test process
cosmicexplorer Sep 5, 2018
8f1a49d
a clean-all shouldn't be necessary here...
cosmicexplorer Sep 5, 2018
7ba39fd
remove everything but the integration test shard we really need
cosmicexplorer Sep 5, 2018
a451b3f
Revert "remove everything but the integration test shard we really need"
cosmicexplorer Sep 5, 2018
9b3bdad
Revert "reduce test shards to the failing ones"
cosmicexplorer Sep 5, 2018
4759e60
skip failing test
cosmicexplorer Sep 5, 2018
52bf2c8
add another TODO describing a hack introduced into this PR
cosmicexplorer Sep 5, 2018
10cfd9b
banish the offending bin target from the command line
cosmicexplorer Sep 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion tests/python/pants_test/backend/python/tasks/test_python_distribution_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,8 @@ def test_python_distribution_with_setup_requires(self):

def test_pants_requirement_setup_requires_version(self):
"""Ensure that a pants_requirement() can be successfully used in setup_requires."""
pants_run = self.run_pants(['-q', 'run', '{}:bin'.format(self.pants_setup_requires)])
pants_run = self.run_pants(['-q', 'run', '{}:bin'.format(self.pants_setup_requires)],
extra_env={'PEX_VERBOSE': '9'})
self.assert_success(pants_run)
# This testproject prints its own version string here, which is the current pants version plus
# the pants fingerprint (as defined in this project's setup.py).
Expand Down
7 changes: 5 additions & 2 deletions tests/python/pants_test/engine/test_parsers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,8 +301,11 @@ def _assert_valid_output(self, relative_json_value, is_inline):
except AssertionError:
def convert_to_stripped_set(json_str):
no_brackets = json_str.replace('{', '').replace('}', '')
distinct_lines = no_brackets.split(',')
return set(distinct_lines)
# TODO: #6275 appears to have broken this so that the `.strip()` is required now, but
# it's not clear why (why would there suddenly be extra whitespace on either side of each
# line, for multiple tests, where there wasn't before?).
distinct_lines = set(l.strip() for l in no_brackets.split(','))
return distinct_lines

self.assertEqual(convert_to_stripped_set(expected), convert_to_stripped_set(output))

Expand Down
5 changes: 3 additions & 2 deletions tests/python/pants_test/projects/base_project_integration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ class ProjectIntegrationTest(PantsRunIntegrationTest):
:API: public
"""

def pants_test(self, command):
def pants_test(self, command, extra_env=None):
"""
:API: public
"""
return self.run_pants(['test'] + command)
return self.run_pants(['test'] + command,
extra_env=extra_env)
8 changes: 7 additions & 1 deletion tests/python/pants_test/projects/test_testprojects_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ def targets(self):
'testprojects/src/java/org/pantsbuild/testproject/thriftdeptest',
# TODO(Eric Ayers): I don't understand why this fails
'testprojects/src/java/org/pantsbuild/testproject/jvmprepcommand:compile-prep-command',
# TODO(#6455): this produces a resolution error in (currently) the test_shard_6() method with
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not totally sure, but I'm pretty damn sure, the issue here is allow_prereleases. The native dists are always prereleases as things stand with their fingerprint version numbering scheme. Pants via pex (like pip), does not resolve prereleases by default - this has to be turned on. I think you'll need come up with a semi-substantial change to Pants' pex building code / PythonRequirement to allow for isolated resolves of only certain requirements with prereleases even if the overall resolve mode is the default of allow_prereleases=False. I dug a bit this am because hello_again resolution failures were blocking my git commit via the lint hook failing. I think it makes sense to uninstall this task pipeline by default until its working reliably. If twitter needs it installed, that could be done with a site-specific plugin register.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK - this was tied up in a fairly subtle dance:

  1. allow_prereleases
  2. leaking of pantsbuild.pants prereleases from tests into pants requirement resolution cache via:

    pants/tests/python/pants_test/backend/python/pants_requirement_integration_test_base.py

    Line 43 in 46d5cb9

    def create_unstable_pants_distribution(self):
  3. egg_info --tag-build=+[rest] interacting badly when applied to a prerelease version.

I'll file here shortly with all the details.

Copy link
Contributor Author

@cosmicexplorer cosmicexplorer Sep 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I suspected something was funky when the version string had the + turned into an _ in the error message. I thought I remembered hearing that allow_prereleases was slightly different than the result of using the --tag-build option from someone somewhere, but I don't recall exactly -- it would make sense if "prereleases" is a superset of tagged builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, uninstalling this task pipeline (the native backend as well as the python_dist() support) by default sounds great, and is the focus of #5970, which I was going to get to after this and another open PR got merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. #5970 is about contrib - I meant more - no matter where this is housed, have it turned off by default, even in pantsbuild/pants - until it is stable. It's caused more than its fair share of thrash due to instability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I also didn't make it clear that my intention with #5970 was also absolutely to turn these off by default (I already wanted to do this just because downloading a huge binary compiler distribution alone feels like something I would want to have to opt in to).

# "Could not satisfy all requirements for hello_again==...", but the missing requirement is
# the one we're trying to satisfy, from the setup.py for the python_dist() target in the same
# directory!
'examples/src/python/example/python_distribution/hello/pants_setup_requires:bin',
]

# Targets that are intended to fail
Expand Down Expand Up @@ -122,7 +127,8 @@ def targets_for_shard(self, shard):
def run_shard(self, shard):
targets = self.targets_for_shard(shard)
pants_run = self.pants_test(targets + ['--jvm-platform-default-platform=java7',
'--gen-protoc-import-from-root'])
'--gen-protoc-import-from-root'],
extra_env={'PEX_VERBOSE': '9'})
self.assert_success(pants_run)

def test_self(self):
Expand Down