https://pantsbuild.org/ logo
#development
Title
# development
h

hundreds-breakfast-49010

07/12/2019, 6:14 PM
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

witty-crayon-22786

07/12/2019, 6:17 PM
not sure i understand the question... i think the description already mentions that
what is your concern?
h

hundreds-breakfast-49010

07/12/2019, 6:28 PM
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

witty-crayon-22786

07/12/2019, 6:29 PM
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

hundreds-breakfast-49010

07/12/2019, 6:32 PM
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

witty-crayon-22786

07/12/2019, 8:33 PM
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

hundreds-breakfast-49010

07/12/2019, 9:11 PM
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

witty-crayon-22786

07/12/2019, 9:13 PM
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

hundreds-breakfast-49010

07/12/2019, 9:18 PM
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

witty-crayon-22786

07/12/2019, 9:23 PM
to share a mutable thing, you need an
Arc<Mutex<T>>
h

hundreds-breakfast-49010

07/12/2019, 9:25 PM
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

witty-crayon-22786

07/12/2019, 9:31 PM
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

hundreds-breakfast-49010

07/12/2019, 9:34 PM
yeah, or it might be enough to have a HashSet or something
although then I'd have to implement Hash on the type
w

witty-crayon-22786

07/12/2019, 9:34 PM
i'm going to guess that
Mutex
doesn't implement Hash
yuuuup
h

hundreds-breakfast-49010

07/12/2019, 9:34 PM
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

witty-crayon-22786

07/12/2019, 9:36 PM
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

hundreds-breakfast-49010

07/12/2019, 9:36 PM
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

witty-crayon-22786

07/12/2019, 9:37 PM
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

hundreds-breakfast-49010

07/12/2019, 9:42 PM
in
Scheduler::execute
?
oh so maybe the registration/deregistration with the logger should happen there
w

witty-crayon-22786

07/12/2019, 9:52 PM
yes, definitely
h

hundreds-breakfast-49010

07/13/2019, 1:21 AM
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

witty-crayon-22786

07/13/2019, 2:52 AM
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?