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

fast-nail-55400

06/17/2020, 8:49 PM
are there good examples in the pants source of wrapping some code in a workunit that someone could point me at?
Is the basic pattern to obtain a WorkunitStore via
context.workunit_store
and then use
start_workunit
and
complete_workunit
?
w

witty-crayon-22786

06/17/2020, 9:16 PM
cc @hundreds-breakfast-49010
f

fast-nail-55400

06/17/2020, 9:17 PM
I think i figured it out. sending PR.
h

hundreds-breakfast-49010

06/17/2020, 9:18 PM
@fast-nail-55400 yeah, look at where we use the
with_workunits
function defined in
workunit_store/src/lib.rs
f

fast-nail-55400

06/17/2020, 9:18 PM
(the PR doesn’t use that
with_workunits
helper. Should it?)
h

hundreds-breakfast-49010

06/17/2020, 9:21 PM
I"m looking. but I don't think it needs to
there's other code in nodes.rs that avoids it
f

fast-nail-55400

06/17/2020, 9:22 PM
I assume that
scope_task_workunit_state
is to scope panics?
h

hundreds-breakfast-49010

06/17/2020, 9:26 PM
it's something pertaining to using workunits in futures contexts that I don't fully understand. @witty-crayon-22786 added that code
w

witty-crayon-22786

06/17/2020, 9:27 PM
task
here is the rust async equivalent of a thread
in particular, a
task
is a tokio task.
that scope call is to propagate the “parent” workunit information along, so that if we create child workunits, they have parents.
so… yes, it should be used if there is a possibility that you’re “expecting to have children” (heh)
h

hundreds-breakfast-49010

06/17/2020, 9:29 PM
this would be good to add as documentation comments in the workunit crate
w

witty-crayon-22786

06/17/2020, 9:29 PM
it’s early days for this. we could use better syntax.
@hundreds-breakfast-49010: it’s there, probably…
h

hundreds-breakfast-49010

06/17/2020, 9:30 PM
I think there's definitely some more polish we could add there
f

fast-nail-55400

06/17/2020, 9:30 PM
seems like I should use with_workunit though and not use start_workunit/complete_workunit directly as I did in the PR
👍 1
w

witty-crayon-22786

06/17/2020, 9:31 PM
@hundreds-breakfast-49010: …yea, nevermind. there isn’t much explanation in there. sorry about that.
that’s the safe bet, yea.
h

hundreds-breakfast-49010

06/17/2020, 9:31 PM
@fast-nail-55400 yeah. I think that would be a reasonable refactor
ideally we would only expose
with_workunit
. there's only one other place in the code where we actually create workunits without using that helper, in node.rs
1
w

witty-crayon-22786

06/17/2020, 9:32 PM
do we need to though?
h

hundreds-breakfast-49010

06/17/2020, 9:33 PM
well it's not strictly necessary; in the sense that the code works as it. but it is confusing that the workunit create exposes two slightly different APIs for doing the same thing
w

witty-crayon-22786

06/17/2020, 9:33 PM
… yea, i guess it does.
h

hundreds-breakfast-49010

06/17/2020, 9:33 PM
in the only two places we actually define workunits (before tom's commit anyway)
w

witty-crayon-22786

06/17/2020, 9:33 PM
@hundreds-breakfast-49010: the nodes.rs stuff needs to add metadata to the workunit i think, which you’d need an api for in
with_workunit
. but yea, could call it a low-level API.
h

hundreds-breakfast-49010

06/17/2020, 9:34 PM
anyway I think this is polish, and not polish that needs to be prioritized right now. that said @fast-nail-55400 if you want to refactor your PR to using
with_workunit
, you might as well
w

witty-crayon-22786

06/17/2020, 9:34 PM
ok, making progress on this other thing: checking back out of this thread. sorry for not adding docs when i was in there =(
h

hundreds-breakfast-49010

06/17/2020, 9:34 PM
although I would've ack'd it regardless
w

witty-crayon-22786

06/17/2020, 9:34 PM
(ping again if need be!)
h

hundreds-breakfast-49010

06/17/2020, 9:34 PM
no worries @witty-crayon-22786 I could've also added more documentation when I've poked around in that code
f

fast-nail-55400

06/17/2020, 9:35 PM
with_workunit consumes the WorkunitStore which means I can’t use it more than once?
or do I need to clone the WorkunitStore?
h

hundreds-breakfast-49010

06/17/2020, 9:35 PM
it's cheaply-clonable
the struct defines a single boolean and an
inner
member wrapped in an
Arc<Mutex<_>>
so just calling
.clone()
on a
WorkunitStore
is a (relatively) cheap operation. the cost comes from interacting with the
Arc<Mutex<_>>
when you call methods on
WorkunitStore
that get proxied to
inner
f

fast-nail-55400

06/17/2020, 9:39 PM
👍