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 --print-exception-stacktrace not invalidating pantsd #10451

Merged
merged 3 commits into from Jul 27, 2020

Conversation

Eric-Arellano
Copy link
Contributor

Before, if a command failed and you run it again with --print-exception-stacktrace, it would make no difference because the daemon's cache was being used.

Now, we force pantsd to restart so that the option works properly.

[ci skip-rust-tests]

# Rust tests will be skipped. Delete if not intended.
[ci skip-rust-tests]
@coveralls
Copy link

coveralls commented Jul 26, 2020

Coverage Status

Coverage remained the same at 0.0% when pulling 7616485 on Eric-Arellano:fix-pantsd into 9ca7b52 on pantsbuild:master.

@@ -572,7 +572,8 @@ def register_bootstrap_options(cls, register):
"--print-exception-stacktrace",
advanced=True,
type=bool,
fingerprint=False,
default=False,
daemon=True,
help="Print to console the full exception stack trace if encountered.",
Copy link
Sponsor Member

@stuhood stuhood Jul 26, 2020

Choose a reason for hiding this comment

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

See #10035 for the explanation of when fingerprint=True vs daemon=True is relevant, but: afaict exceptions before the scheduler is created should always result in a full stacktrace, so it shouldn't be necessary to restart pantsd (ie, daemon=True) for this option.

On the other hand, having this marked fingerprint=False means that we won't recreate the Scheduler if this value changes... and that is a bug, because the Scheduler does consume this option. So I think that you probably just want to mark this fingerprint=True (which should be equivalent to removing the argument).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. This morning I was having issues with fingerprint=True actually causing a change in behavior - it did change the behavior, but it looked like portions of the stacktrace were still being printed (less than normal). I didn't capture that output, sorry.

I can't reproduce it though, so looks like this is working correctly now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't reproduce it though

That would be because I changed the wrong option. Oops.

This is the output I get when fingerprint=True, daemon=False. This was after running with --print-exception-stacktrace already.

▶ ./pants --no-pantsd --no-print-exception-stacktrace lint src/python/pants/util/strutil.py
17:18:34 [ERROR] 1 Exception encountered:

17:18:41 [ERROR] 1 Exception encountered:

  ValueError:
Traceback (most recent call last):
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/bin/local_pants_runner.py", line 288, in run
    engine_result = self._run_v2()
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/bin/local_pants_runner.py", line 176, in _run_v2
    return self._maybe_run_v2_body(goals, poll=False)
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/bin/local_pants_runner.py", line 199, in _maybe_run_v2_body
    poll_delay=(0.1 if poll else None),
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/init/engine_initializer.py", line 135, in run_goal_rules
    goal_product, params, poll=poll, poll_delay=poll_delay
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/engine/internals/scheduler.py", line 558, in run_goal_rule
    self._raise_on_error([t for _, t in throws])
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/engine/internals/scheduler.py", line 528, in _raise_on_error
    wrapped_exceptions=tuple(t.exc for t in throws),
pants.engine.internals.scheduler.ExecutionError: 1 Exception encountered:

  ValueError:

If I have daemon=True, I get the expected result:

▶ ./pants --no-pantsd --no-print-exception-stacktrace lint src/python/pants/util/strutil.py
17:18:34 [ERROR] 1 Exception encountered:

  ValueError:

(Use --print-exception-stacktrace to see more error details.)

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Hm. It looks like that's because this is a sneaky global-mutable option on the ExceptionSink:

class ExceptionFormatter(Formatter):
"""Uses the `--print-exception-stacktrace` option to decide whether to render stacktraces."""
def formatException(self, exc_info):
if ExceptionSink.should_print_exception_stacktrace:
return super().formatException(exc_info)
return "\n(Use --print-exception-stacktrace to see more error details.)"
... (which I added. *sigh) would you mind adding a comment/TODO explaining that and setting daemon=True for now?

# Rust tests will be skipped. Delete if not intended.
[ci skip-rust-tests]
Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

With the comment, please. Thanks, and sorry for the trouble!

# Rust tests will be skipped. Delete if not intended.
[ci skip-rust-tests]
@stuhood stuhood merged commit d63b29c into pantsbuild:master Jul 27, 2020
@Eric-Arellano Eric-Arellano deleted the fix-pantsd branch July 27, 2020 04:47
Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Jul 27, 2020
…d#10451)

Before, if a command failed and you run it again with `--print-exception-stacktrace`, it would make no difference because the daemon's cache was being used.

Now, we force pantsd to restart so that the option works properly.

[ci skip-rust-tests]# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
stuhood pushed a commit that referenced this pull request Jul 27, 2020
…k of #10451) (#10461)

Cherry-pick of d63b29c.

[ci skip-rust-tests]
[ci skip-jvm-tests]
@Eric-Arellano Eric-Arellano removed this from the 1.30.x milestone Aug 5, 2020
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