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

happy-kitchen-89482

01/08/2020, 5:43 PM
Is
TransitiveHydratedTargets.closure
in topological order? What is the canonical way to get the dep closure of some targets in t-order?
w

witty-crayon-22786

01/08/2020, 5:56 PM
it is not, no
i don't think there is one... executing recursively is supposed to be more natural in the engine
although that hasn't been proven in anything other than prototypes
h

happy-kitchen-89482

01/08/2020, 6:36 PM
Yeah, this is a case where I actually need the graph in order, not directly for execution purposes.
w

witty-crayon-22786

01/08/2020, 6:38 PM
closure is ... depth first, i think? can confirm
why topological, if not for execution?
h

happy-kitchen-89482

01/08/2020, 9:00 PM
Need it to order the ExportedTargets, which I guess eventually may affect execution order, but it's not the same as target closure dep order (although it is induced by it)
I mean we do need it in order to process the ExportedTargets in the right order, but note that those targets may not even be in the closure of the cmd-line specs
It might be possible to do something without an explicit sort in hand though, now that I think of it
w

witty-crayon-22786

01/08/2020, 9:04 PM
well, you indicated that execution order is actually relevant there, and i suggested recursion to allow for parallelism
recursion with actual declared dependencies both allows for parallelism, and avoids explicit ordering.
h

happy-kitchen-89482

01/08/2020, 9:05 PM
Yes, but this is a tricky case because the things we're recursing over might not be in the dep closure (they are the targets that own them)
It might be OK to recurse on "run setup py on the owner of each of my deps".
w

witty-crayon-22786

01/08/2020, 9:06 PM
right.
h

happy-kitchen-89482

01/08/2020, 9:06 PM
It might meet the same owner multiple times, but that should be fine because the engine will have memoized it
So maybe that's fine
w

witty-crayon-22786

01/08/2020, 9:06 PM
yep
h

happy-kitchen-89482

01/08/2020, 9:07 PM
There's a caveat
w

witty-crayon-22786

01/08/2020, 9:09 PM
(the engine will give you a useful cycle error if cycles exist)
h

happy-kitchen-89482

01/08/2020, 9:32 PM
The caveat is if the user asked to run on specific exported targets, do we just do those (in order) or do we say "if you want to enforce order, you have to run on all relevant exported targets in order, which may be more than the ones you asked for"
I think that's fine
w

witty-crayon-22786

01/08/2020, 9:35 PM
Well, recursing to do "something" is fine, even if it isn't side-effecting.
Ideally this would not be side-effecting.
h

happy-kitchen-89482

01/08/2020, 9:37 PM
Oh I just mean, if the only way to enforce order is to recurse then we are committing to "if order matters, we will run not just on the specific exported targets you asked for, but on all of the relevant ones"
But I think that's fine
the reason order might matter also implies that we should do that
i.e., so you don't publish dependees before their dependencies
Oh but then there are issues with writing to the console.
Hmm.
I guess we just don't write to the console then.
Can a console rule recurse on itself?
w

witty-crayon-22786

01/08/2020, 10:08 PM
no
the pattern that exists in
v1
is to have the roots be the things that were actually requested, and thus published to
dist
for example:
./pants bundle $roots
only published the roots to
dist
...even though other bundles might be privately built under
.pants.d
in order to accomplish that work
the same goes for writing to the console***.
h

happy-kitchen-89482

01/08/2020, 10:10 PM
Right
but here there is a correctness issue
Possibly
We may have to act on non-roots
and if we do this recursively then we can't, for example, use the Workspace to write the results to dist/
w

witty-crayon-22786

01/08/2020, 10:12 PM
sure. but see above. if they weren't roots then you shouldn't.
h

happy-kitchen-89482

01/08/2020, 10:12 PM
Sure, that is the case today
but it leads to a possible correctness problem
w

witty-crayon-22786

01/08/2020, 10:12 PM
...why?
h

happy-kitchen-89482

01/08/2020, 10:13 PM
If the setup-py command we run publishes to pypi, then we want to publish all dependencies before we publish the roots
w

witty-crayon-22786

01/08/2020, 10:13 PM
sure. but that's not related to the dist directory or the console
h

happy-kitchen-89482

01/08/2020, 10:13 PM
So it's true that technically that publish doesn't require writing to
dist
I guess
But that is pretty unintuitive
w

witty-crayon-22786

01/08/2020, 10:14 PM
the side-effects-in-rules part is the un-intuitive part, i think.
h

happy-kitchen-89482

01/08/2020, 10:14 PM
Yes, publishing to PyPI is one hell of a side effect...
w

witty-crayon-22786

01/08/2020, 10:15 PM
if possible, factoring this as "build all the artifacts, and then have the console_rule do the actual publishing" would be better.
i mean, in pantsbuild's usage of pypi, we do that via twine, right?
ditto ivy/maven publishing: prepare all of the things that you might upload, and then attempt to atomically upload at the end.
h

happy-kitchen-89482

01/08/2020, 10:22 PM
Well, we don't upload ourselves
the setup.py command does
w

witty-crayon-22786

01/08/2020, 10:22 PM
no
twine does.
h

happy-kitchen-89482

01/08/2020, 10:23 PM
well, may or may not
w

witty-crayon-22786

01/08/2020, 10:23 PM
sure. in this case, architecturally, discouraging that would be good.
h

happy-kitchen-89482

01/08/2020, 10:25 PM
You can upload with
sdist upload
as your setup.py command
build and upload, that is
We can disallow this I suppose
Twine is definitely a better way to do this
but I don't know that we can or should prevent people from doing this the distutils way
w

witty-crayon-22786

01/08/2020, 10:27 PM
we should, probably.
h

happy-kitchen-89482

01/08/2020, 10:28 PM
We don't provide an alternative
Other than "here's everything in dist/, now you figure out how to upload it"
which is actually fine with me, for now, as I think about it.
w

witty-crayon-22786

01/08/2020, 10:28 PM
er... i pointed to one above? you can invoke twine for them
i would call it the first implementation of a
publish
goal, parallel to a future JVM v2 publish using ivy.
h

happy-kitchen-89482

01/08/2020, 10:31 PM
you can invoke twine for them outside of pants, but that's not really us providing a solution
For example we're not helping solve the dependency order issue, you'd have to do that by ordering your twine invocations somehow
But yeah we can create a
publish
goal that invokes twine and sorts that out
And then basically the setup-py goal shouldn't care about order, and we just have to recommend or enforce that it doesn't attempt to publish
❤️ 1
w

witty-crayon-22786

01/08/2020, 10:33 PM
you can invoke twine for them as a foreground interactive process.
you're right that ordering would matter in that case.
yea, that sounds good.