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

Issues/8786 #9074

Closed
wants to merge 42 commits into from
Closed

Issues/8786 #9074

wants to merge 42 commits into from

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Feb 5, 2020

An attempt to get John's pex 2.x upgrade working while he's OOO. Do not review.

try:
setup_runner.run_setup_command(source_dir=setup_dir, setup_command=split_command)
except SetupPyRunner.CommandFailure as e:
raise TaskError(f'Install failed: {e}')
Copy link
Member

Choose a reason for hiding this comment

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

nit: {e!r}
#8178 (comment)

Copy link
Contributor

@jsirois jsirois Feb 18, 2020

Choose a reason for hiding this comment

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

I've heard this recommendation before from Eric and I don't understand it. In particular, a TaskError is user facing and the messages are printed to the console even with tracebacks disabled. In those cases adding the exception type to the message - which is all repr does afaict, seems generally undesirable. An internal name leaked to a user is not helpful. I'ts also not very helpful to a debugging dev with bactraces turned off. With backtraces turned on, the backtrace is what is helpful.

Concretely in this case, on the CLI the user would currently see:

$ ./pants setup-py ...
...
Install failed: Failed to execute python /a/pex setup.py ...

With repr they'd see:

$ ./pants setup-py ...
...
Install failed: CommandFailure('Failed to execute python /a/pex setup.py ...

I'm not seeing what the point of adding CommandFailure('...') wrapping the message is.

manylinux = self.get_options().resolver_manylinux
if manylinux.lower() in ('false', 'no', 'none'):
if self.get_options().resolver_use_manylinux:
logger.warning('The [{scope}] manylinux option is explicitly set to {manylinux} '
Copy link
Member

Choose a reason for hiding this comment

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

nit: f-strings.

except SetupPyRunner.CommandFailure as e:
raise self.BuildLocalPythonDistributionsError(
f"Installation of python distribution from target {dist_tgt} into directory "
f"{dist_target_dir} failed using the host system's compiler and linker: {e}")
Copy link
Member

Choose a reason for hiding this comment

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

same nit, {e!r}

@jsirois
Copy link
Contributor

jsirois commented Feb 19, 2020

The remaining test failures are covered by pex-tool/pex#892

@jsirois
Copy link
Contributor

jsirois commented Feb 20, 2020

I've refreshed against upstream master and applied the pex 2.1.4 fixes. If this goes green I may switch back to #8881 to clean up and finish.

Interpreter constraints were too broad for the default pytest setup we use
which pins zipp==2.1.0 which only works for python>=3.6.

Also kill legacy constraints passed to the `pants_run` IT helper.
Keep Pants to its baileywick - use pex to do this.
@jsirois
Copy link
Contributor

jsirois commented Feb 20, 2020

Alright, the 1 failing shard - Integration tests - V2 (Python 3.6) - is now green.

The Self-checks and lint (Python 3.6) shard consistently times out though and individual ~random tests time out in Integration tests - V2 (Python 3.6), forcing ~3 re-runs to get green. Looking into these timeouts now.

@jsirois
Copy link
Contributor

jsirois commented Feb 20, 2020

OK - the lint shard is unrelated and being tackled in #9155.

@jsirois
Copy link
Contributor

jsirois commented Feb 21, 2020

Ok the 42e38a7 merge pulls in Eric's lint CI shard timeout fix and so should nominally go green. Still spelunking very flaky Integration tests - V2 (Python 3.6) shard timeouts.

Since this is now done in PythonBinary internally, we can revert to the
cleaner check. Additionally, this self-contained handling now removes
the need for PythonInterpreter cache clearing in
test_select_interpreter.py.
@jsirois
Copy link
Contributor

jsirois commented Feb 21, 2020

Alright - both 42e38a7 and 6d4f506 took 1 round of shard retries to converge to green. That seems more in-line with the status quo. I'll work on cleaning this all up and probably will do that over in #8881 or a new PR.

Thanks again @benjyw for all your help here.

@jsirois
Copy link
Contributor

jsirois commented Feb 21, 2020

Thanks @benjyw and @asherf - the changes here have been applied back to #8881
I'm going to close this PR to move finish-up back there.

@jsirois jsirois closed this Feb 21, 2020
@benjyw benjyw deleted the issues/8786 branch July 15, 2020 21:25
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.

3 participants