-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Add UI to engine execution #6647
Changes from 25 commits
8da78d7
b287219
f22b2a3
605b788
cb99460
9a8d71b
9c288cf
df8849a
60700e8
c2b7e2a
2fcfd7d
a65e348
c67490a
298d821
956464f
8ac61be
bff4738
3f3caf1
fb09b8a
3ec95d2
75a7474
2a3a45f
07d4eb5
32547fb
df64776
61a3a76
d9be623
96c39bc
0c95fd9
a89dabc
ed7d4d0
1b77e56
d4057e9
73679c6
ac9741d
bb1839a
a66be04
446370b
7739761
a7aa106
bc87c72
3c09ec8
a3500fe
dda77fd
725cf8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,3 +86,4 @@ resettable = { path = "resettable" } | |
smallvec = "0.6" | ||
tokio = "0.1" | ||
tempfile = "3" | ||
ui = { path = "ui" } |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -244,7 +244,7 @@ impl EngineDisplay { | |||||||||||||||||
|
||||||||||||||||||
fn render_for_pipe(&self) { | ||||||||||||||||||
// TODO: Handle non-tty output w polling interval adjustment + summary rendering. | ||||||||||||||||||
panic!("TODO"); | ||||||||||||||||||
// Nothing needs to be printed to pipe by default. | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, while this definitely shouldn't panic, I think that rather than disabling this at this level, we should ensure that when we're inspecting the "should we render the UX" global option, we check whether we're in a tty, and then we explicitly disable the UI rendering if we do not have a TTY. This information is passed from the pantsd client to the pantsd server via: https://github.com/pantsbuild/pants/blob/master/src/python/pants/java/nailgun_protocol.py#L285-L314 ... but I think that the gist is still that looking at Having done that, we should maybe just remove this method entirely if possible...? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should've clarify a bit that this is not required for this change. Just that I don't want pants to crash on pipe. There is already logic above it which does not call this function at all if it is tty. pants/src/rust/engine/ui/src/display.rs Lines 261 to 268 in 298d821
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. But if my understanding is correct, we should just remove support for attempting to render to a pipe entirely? Maybe that can be a followup ticket: to basically just have the |
||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// Paints one screen of rendering. | ||||||||||||||||||
|
@@ -309,6 +309,10 @@ impl EngineDisplay { | |||||||||||||||||
|
||||||||||||||||||
// Terminates the EngineDisplay and returns the cursor to a static position. | ||||||||||||||||||
pub fn finish(&mut self) { | ||||||||||||||||||
// Don't do anything if it's not tty. | ||||||||||||||||||
if !self.is_tty { | ||||||||||||||||||
return; | ||||||||||||||||||
} | ||||||||||||||||||
self.running = false; | ||||||||||||||||||
let current_pos = self.get_cursor_pos(); | ||||||||||||||||||
let action_count = self.action_map.len() as u16; | ||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on #6654, I think that you should add a
@console_rule
specific method here, and remove the TODO that Daniel added:pants/src/python/pants/engine/scheduler.py
Lines 541 to 542 in 3d22e95
The idea would be that: a call to
product_request(s)
would never render the UI, but a call to this new method would, and would also address that TODO. It would also not support batching likeproduct_requests
(plural) does.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a new method specifically for console rules. However it is not clear to me whether the entire
if
statement was specifically for console rules, thus can be removed?pants/src/python/pants/engine/scheduler.py
Lines 591 to 613 in 1b77e56