https://pantsbuild.org/ logo
Join Slack
Powered by
<https://github.com/pantsbuild/pants/issues/7906> ...
# development
h
https://github.com/pantsbuild/pants/issues/7906 <- asked a few questions about how to go about tackling this issue @witty-crayon-22786 @red-balloon-89377
w
not sure i understand the question... i think the description already mentions that
what is your concern?
h
in 
<http://scheduler.rs|scheduler.rs>
, which looks like the only place we create a 
EngineDisplay
, should I be immediately getting a mutable reference to the logger singleton and adding a reference to that newly-created 
EngineDisplay
in that code?
that seems, I dunno, a little kludgy
also with that strategy I don't think there's any way to get rid of an 
EngineDisplay
reference from 
Logger
when no longer needed, which will probably cause a borrow check error when I actually code this up
w
yes, that's what it is suggesting. other ideas definitely welcome! but the engine display is only valid for the length of that block, if the ui is enabled.
and a reminder that the way to test out the UI in particular is to do something like:
Copy code
./pants --no-v1 --v2 --v2-ui test tests/python/pants_test/util:dirutil
the UI display tramples logging, which is the reason for this ticket
the UI is enabled for a while, and then disabled at the end of that block. if you have other cleanup ideas they would be welcome
h
so basically calling 
EngineDisplay::log
is a curses-ui-aware way of logging to console
which implies that if the global logger has a reference to an 
EngineDisplay
we want to avoid calling the existing logging functionality
and only call 
EngineDisplay::log
which I guess woudl have to happen in the 
log
function on the 
Log
trait that 
Logger
implements
basically, it's a new 
Destination
, right?
w
re: destinations... um, unsure
the EngineDisplay will always be a client, and will never be pantsd
so you would not want to log anything bound for pantsd to the EngineDisplay
h
so, maybe, iff the current destination is 
Destination::Stderr
, 
Logger
should also log to every one of its list of 
EngineDisplay
(which might be empty)
and if the destination is 
Pantsd
, just ignore that
i also notice that 
EngineDisplay::log
expects a string but 
Logger
expects to deal with a more complicated 
Record
data structure
but that just wraps a string with some metadata, so maybe all that metadata can get ignored for 
EngineDisplay
w
i believe so. i think the Logger API requires that the implementation do some filtering (ie "i'm at level INFO, so don't log things below that"), but i'm not sure where that happens.
h
hm, 
EngineDisplay
requires a 
&mut self
reference to call its 
.log()
method, but the 
Log
trait requires that 
log
be 
&self
I don't think there's a way that I can store a mutable reference to a 
EngineDisplay
on a 
Logger
I only have an immutable reference ot
w
to share a mutable thing, you need an 
Arc<Mutex<T>>
h
so, I'd want a field of type 
Vec<Arc<Mutex<EngineDisplay>>>
on 
Logger
, and then I can call methods on the Arc<Mutex<_>> to get me a mutable reference to it
w
yup
and you'd need to change 
<http://scheduler.rs|scheduler.rs>
in the same way, because the EngineDisplay would now be a thing shared between multiple threads
re: 
Vec<Arc<Mutex<EngineDisplay>>>
... it's possible that because you need to be able to add at the beginning and remove at the end, you might make it a 
Map<SomeUniqueIdThingie, Arc<Mutex<EngineDisplay>>>
instead
or maybe not... could walk through the Vec and look at all of them if they have some other uniquely identifying info, i suppose
h
yeah, or it might be enough to have a HashSet or something
although then I'd have to implement Hash on the type
w
i'm going to guess that 
Mutex
doesn't implement Hash
yuuuup
h
ah so I'd need a map from some kind of unique id anyway
so, 
EngineDisplay
gets created in 
Session::new
and I assume only gets destroyed in `Session`'s implicit Drop
w
probably makes sense for the logger to choose the id. so something like :
Copy code
fn add_engine_display(Arc<Mutex<EngineDisplay>>) -> SomeUniqueId;

fn remove_engine_display(SomeUniqueId);
h
so 
Session
should be in charge of making sure that the global logger gets registered with a new 
EngineDisplay
on creation, and loses it on destruction
w
ehhh, not sure about that.
it's only "active" in that single method in 
<http://scheduler.rs|scheduler.rs>
it might live longer than that, but that's the only time it should be receiving anyuthing.
h
in 
Scheduler::execute
?
oh so maybe the registration/deregistration with the logger should happen there
w
yes, definitely
h
have a draft PR: https://github.com/pantsbuild/pants/pull/8049/commits/7dc0cfb190ac436a122e295150f3a5986cfa5ca6 , but I'm having trouble wrangling the wrapped types in the way I need to to get everything compiling
in 
<http://scheduler.rs|scheduler.rs>
I think I can only get access to the unlocked 
EngineDisplay
via 
MutexGuard
, which is the wrapper (which derefmut's to 
&mut EngineDisplay
) from 
.lock()
on a Mutex
but that means that I can only store basically a mut pointer to the 
EngineDisplay
in the 
LOGGER
singleton, which has lifetime problems
maybe there's a way to work around that? but I can't think of one off the top of my head
w
Mutexes in rust own the things they are holding, so that is not surprising I don't think
Either consumer will need to acquire the mutex to access the thing. And that should be fine here?
Open in Slack
PreviousNext
Powered by