pantsbuild #development

Channels

welcome

github-notifications

development

average-vr-56795

11/28/2019, 5:39 PM

Yeah, the listing members of the union isn’t the scary part, and even the ordering isn’t too crazy (though would need an API which wouldn’t necessarily be pretty). The scary piece is the mutably overwriting

Target

details.

aloof-angle-91616

11/28/2019, 5:40 PM

oh, i was thinking we'd introduce some intermediate type instead of dealing with

Target

directly

average-vr-56795

11/28/2019, 5:40 PM

We currently want the following to work, I think:

./pants fmt test some:target

Right now in v1 I think it does, and in v2 it doesn’t -

test

will operate on the original sources before formatting.

aloof-angle-91616

11/28/2019, 5:40 PM

ah, ok, thank you for describing that

average-vr-56795

11/28/2019, 5:41 PM

(Personally, I’d rather just not allow multiple `@console_rule`s in an invocation, or not allow mutability of the buildroot within an invocation, but I think I’m in a pretty small minority in those things…)

aloof-angle-91616

11/28/2019, 5:41 PM

i'm pretty ok with not allowing multiple `@console_rule`s at once tbh

average-vr-56795

11/28/2019, 5:43 PM

On the mutability thing - I think we currently have an interesting situation with pantsd where (depending on a race condition) we’ll actually invoke every formatter twice for every target, but the second run will no-op (as long as the formatter is deterministic), because when we write the update, we’ll get a watchman event, decide we need to re-run the rule graph, and re-invoke the formatter, which hopefully won’t change the file again

aloof-angle-91616

11/28/2019, 5:47 PM

i would like to be able to not trust that filesystem events occur in any particular order, i.e. if watchman sends a spurious notification i'd like to be ok with that. in the above case, i feel like idempotent (or at least very quickly converging) formatters are something we can try to have though? i don't necessary like the idea of the double-check you're describing, but i'm not sure i can see an alternative that would do something to stop the watchman event that i like.

aloof-angle-91616

11/28/2019, 5:47 PM

i think @wide-energy-11069 was seeing this with the

--v1-loop

PR too and had an explicit lock in python to overcome it, which was really interesting

aloof-angle-91616

11/28/2019, 5:48 PM

https://github.com/pantsbuild/pants/pull/8640 for context

wide-energy-11069

11/28/2019, 5:49 PM

It won't solve the fmt issue

😂 1

aloof-angle-91616

11/28/2019, 5:49 PM

yeah

average-vr-56795

11/28/2019, 5:49 PM

My personal answer is that things inside pants should never mutate input files, and if you want to modify things you should either: 1. From a wrapper script, run some kind of pants query to get the information you need to run commands, and then run those commands outside of pants, or 2. Ask pants to do the thing for you outputting to a temporary directory (like https://github.com/pantsbuild/pants/blob/97cddca085d3e436b9f0a67266f208a3ed238d65/src/python/pants/backend/jvm/tasks/rewrite_base.py#L27-L30 supports) and if you want to do the overwriting, do that in a wrapper script / similar

average-vr-56795

11/28/2019, 5:50 PM

But I also see that that’s not super clean or convenient

aloof-angle-91616

11/28/2019, 5:50 PM

^i feel like the above also plays with the

@console_rule

idea more naturally, and also makes it more feasible to use pants on the command line

average-vr-56795

11/28/2019, 5:51 PM

(Basically, I want pants to be a tool with limited scope that plays nicely with others, rather than an entire workflow engine, and one of my lines is mutability 🙂)

aloof-angle-91616

11/28/2019, 5:51 PM

./pants lint my/target:name | tar xvf -

@average-vr-56795 how would you feel about this

aloof-angle-91616

11/28/2019, 5:52 PM

it's functionally the same as like maybe

./pants lint --output-dir=/tmp/123 my/target:name && cp -r /tmp/123/* .

or something (the

cp -r

part is wrong)

aloof-angle-91616

11/28/2019, 5:52 PM

but it also allows streaming output

average-vr-56795

11/28/2019, 5:54 PM

I’m slightly sceptical of the streamingness, because it maybe assumes all the linting is done before output starts? But ish?

aloof-angle-91616

11/28/2019, 5:55 PM

ah, sorry, this is the issue you were mentioning above

aloof-angle-91616

11/28/2019, 5:55 PM

i assumed that the targets would be snapshotted before output starts

average-vr-56795

11/28/2019, 5:56 PM

Yeah, except pantsd/watchman doesn’t know or care that you intend to be doing the overwriting, as (imo) it shouldn’t

aloof-angle-91616

11/28/2019, 5:57 PM

hm, ok

aloof-angle-91616

11/28/2019, 5:57 PM

i'm not clear on how the invalidation works from pantsd/watchman if

--loop

isn't on

aloof-angle-91616

11/28/2019, 5:58 PM

i believe that was related to the node dirtying stuff stu worked on

average-vr-56795

11/28/2019, 5:58 PM

We have code which basically goes “Oh, a file changed, I need to invalidate and re-run the graph, but if that happens more than n times, I’ll give up and error”

👌 1

average-vr-56795

11/28/2019, 5:59 PM

And I like the idea that n could be set to 0 if we wanted (i.e. I don’t think “implicitly do a little retrying” is a hard constraint we should be attached to)

aloof-angle-91616

11/28/2019, 5:59 PM

if we're running a rule that is intended to modify files in the workspace, could we potentially add a lock in rust which queues the file events until after the workspace rule completes?

average-vr-56795

11/28/2019, 6:01 PM

Yeah, we can add synchronisation if we need to (and the fact that we’ve said “mutation should only happen in

@console_rule

s makes it easier if we need), but personally I’d rather just say “no mutation, do your synchronistaion outside of pants” 😛