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 CI failures introduced by #6275 #6454

Merged
merged 12 commits into from Sep 5, 2018

Conversation

Projects
None yet
2 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Sep 5, 2018

Problem

When #6275 was merged to master, it failed two test shards, and these errors weren't spurious.

Solution

  • fixed (ish) the json error (from the py3 shard), and left a detailed TODO next to the workaround
  • skipped the integration test that was failing (test_pants_requirement_setup_requires_version(), introduced in #6275) -- see #6455 for more info.

cosmicexplorer added some commits Sep 5, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Sep 5, 2018

The unit test failure is fixed, working on the integration test failure now.

cosmicexplorer added some commits Sep 5, 2018

@cosmicexplorer cosmicexplorer changed the title [WIP] Fix test failures from #6275 Fix CI failures introduced by #6275 Sep 5, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Sep 5, 2018

If this doesn't pass CI, I will simply need to skip the larger test_shard_6() instead of the smaller test that integration test was failing in. Hopefully this more surgical skip will work.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Sep 5, 2018

...it turned out that failure wasn't about the test failing, it was about ./pants test being called on a python_binary() target, and then something failed. For now, the offending target (examples/src/python/example/python_distribution/hello/pants_setup_requires:bin) has been added to known_failing_targets in test_testprojects_integration.py.

@cosmicexplorer cosmicexplorer merged commit bff18f5 into pantsbuild:master Sep 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -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

This comment has been minimized.

@jsirois

jsirois Sep 5, 2018

Member

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.

This comment has been minimized.

@jsirois

jsirois Sep 5, 2018

Member

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:
  3. egg_info --tag-build=+[rest] interacting badly when applied to a prerelease version.

I'll file here shortly with all the details.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Sep 5, 2018

Contributor

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.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Sep 5, 2018

Contributor

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.

This comment has been minimized.

@jsirois

jsirois Sep 5, 2018

Member

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.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Sep 5, 2018

Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment