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

witty-crayon-22786

08/06/2020, 5:16 PM
@hundreds-father-404, @happy-kitchen-89482: mm. this might be a fun one:
Copy code
pants.backend.python.rules.run_setup_py.AmbiguousOwnerError: Exporting owners for src/python/foo/resources/js/code.js:../resources are ambiguous. Found src/python/foo/foo.py and 1 others: src/python/foo/__init__.py
👀 1
so… yes. there are multiple ancestor targets, because there are multiple ancestor files
perhaps there needs to be a “group by `provides=`” step somewhere…?
h

happy-kitchen-89482

08/06/2020, 5:19 PM
Oh, both those subtargets got the
provides=
copied to them?
w

witty-crayon-22786

08/06/2020, 5:20 PM
yes
so
Targets
are now always expanded into per-file targets, and the base targets are filtered out. meaning that you won’t observe the base targets.
UnexpandedTargets
contains only the base targets, and no subtargets
👍 1
unfortunately, this is not typesafe: in both cases, the underlying type is `Target`… we’re hoping we can clean that up before plugin api stability.
(and
TransitiveTargets
contains expanded per-file targets.)
so yea: no “but…“.
@happy-kitchen-89482: thoughts on how this should act? if you have time to look at that test while i apply Eric’s other feedback, that would be helpful.
👍 1
h

happy-kitchen-89482

08/06/2020, 5:27 PM
Sure, I'll take a look
w

witty-crayon-22786

08/06/2020, 5:27 PM
./pants test src/python/pants/backend/python/rules/run_setup_py_test.py
in https://github.com/pantsbuild/pants/pull/10511
thank you!
h

happy-kitchen-89482

08/06/2020, 5:47 PM
Looking now, but this does seem to expose a problem at the model level - the "provides=" clause really does "belong" to the original target and makes less sense pushed down to the subtargets. For example, we must know the location of the original target in the filesystem, because we only allow it to export code from other targets that are below it in the filesystem.
👍 1
w

witty-crayon-22786

08/06/2020, 5:49 PM
any file still has an owning target, and you could continue to enforce the location of that target
this feels similar to what we will need to do to compile batches of files though… some amount of grouping will definitely be necessary.
in this case, it feels like you might need to either group by the
provides=
, or by the base address/target (
target.address.maybe_convert_to_base_target()
, which actually gives you back the address of the base target)
h

happy-kitchen-89482

08/06/2020, 5:51 PM
Is the dir of the owning target the same as the original target? I thought it was the dir of the file (which could be different if rglobs are used)
w

witty-crayon-22786

08/06/2020, 5:52 PM
the
spec_path
is the location containing the target definition
👍 1
and we continue to enforce that a target is above/at any files that it owns
h

happy-kitchen-89482

08/06/2020, 5:53 PM
Sure, but I'm referring to the fact that if exported target A depends on some other target B, then B must be no higher than A in the filesystem in order to be published in A's dist. So we must know where the original A sat in the filesystem.
So if
spec_path
on the subtarget is the dir of the original target, then we're fine, but is that always the case, even if the files in that target were several levels below the target?
w

witty-crayon-22786

08/06/2020, 5:54 PM
it is always the case.
h

happy-kitchen-89482

08/06/2020, 5:55 PM
Great
w

witty-crayon-22786

08/06/2020, 5:55 PM
the spec_path is always the location of the target.
h

hundreds-father-404

08/06/2020, 5:56 PM
I do wonder if
setup-py
should use solely
UnexpandedTargets
and/or
addr.maybe_convert_to_base_target()
w

witty-crayon-22786

08/06/2020, 5:58 PM
maybe. it does request
TransitiveTargets
right near where this test fails, so we’d possibly need to expose
UnexpandedTransitiveTargets
.
it would be a good sign if it is able to operate on files only though, because this grouping is related to what we’ll need to do for compilation.
h

hundreds-father-404

08/06/2020, 6:02 PM
so we’d possibly need to expose UnexpandedTransitiveTargets
This would allow us to resolve the concern I have about
filedeps --transitive --globs
h

happy-kitchen-89482

08/06/2020, 6:38 PM
I don't see how this is related to compilation?
w

witty-crayon-22786

08/06/2020, 6:38 PM
@happy-kitchen-89482: we will need to detect and group by cycles
to create batches of files to compile.
h

happy-kitchen-89482

08/06/2020, 6:39 PM
OK, so just the concept of grouping. Here cycles aren't the issue.
w

witty-crayon-22786

08/06/2020, 6:39 PM
yea. the concept of grouping is going to be important.
h

hundreds-father-404

08/06/2020, 6:42 PM
We should maybe punt on figuring that out for the sake of 2.0, though. Would the UnexpanddTargets work if we had UnexpandedTransitiveTargets to go with it?
(ik that UnexpandedTransitiveTargets will result in unfortunate code duplication)
w

witty-crayon-22786

08/06/2020, 6:47 PM
let’s see where we are after Benjy has looked at it for a while.
i think that that might be the second to last test failure, i can look at the other one for a bit.
yea, that’s the last test failure. going to apply some more of Eric’s feedback, and then i’ll be busy until 2pm: can talk about what we want to do here then maybe?
👍 1
@happy-kitchen-89482: i was thinking of taking a quick walk… should we schedule some time to talk about it? like 2:30pm?
👍 1
h

happy-kitchen-89482

08/06/2020, 9:16 PM
Sure, I'm here
w

witty-crayon-22786

08/06/2020, 10:09 PM
@hundreds-father-404, @happy-kitchen-89482: pushed… expecting it to go green on this run
🔥 1
❤️ 1
👖 1
h

happy-kitchen-89482

08/06/2020, 10:09 PM
thx
Got this done against master, as it makes sense to merge it separately anyway: https://github.com/pantsbuild/pants/pull/10565
Note that it required no changes to
run_setup_py.py
!
We could possibly tighten up the target type in
ExportedTarget
to be a
PythonDistribution
, but I don't see much need, since the target API is field-centric anyway. The fact that
run_setup_py.py
needed no changes is a strength, not a weakness!
w

witty-crayon-22786

08/06/2020, 11:20 PM
👖 1
🔥 3
❤️ 1
@happy-kitchen-89482: nice!
h

happy-kitchen-89482

08/06/2020, 11:34 PM
OK I lied, just pushed a small change to
run_setup_py.py
itself, so that we do the right thing if there are 3rdparty requirements in the python_distribution itself's dependencies
h

hundreds-father-404

08/06/2020, 11:36 PM
Now that Stu’s change landed, could you check if this fixes the issue he was mentioning, please? I think it might because there are no
sources
for a
PythonDistribution
, so it won’t ever convert into a generataed subtarget
h

happy-kitchen-89482

08/06/2020, 11:54 PM
That's the idea... will test it out
The skipped test passes, two others fail on what looks like something small.
metal 1
Yeah, it's just a failed
Unmatched glob
check, did that get stricter?
w

witty-crayon-22786

08/07/2020, 12:00 AM
shouldn’t have, no.
h

happy-kitchen-89482

08/07/2020, 12:01 AM
Oh I see what happened, nm
easy fix
OK all passes
🎉 1
w

witty-crayon-22786

08/07/2020, 12:04 AM
woot 😃
h

happy-kitchen-89482

08/07/2020, 12:04 AM
pushed fixes and responses to code review
❤️ 1
I like how this turned out. We could have done this particular change a long time ago, but I like that filedeps pushed us to a better design than the one we had.
👍 1
What appeared to be a problem with filedeps was actually a problem with our old model.
💯 1
That bodes well.
Going to take a little break, feel free to merge if you need to, or I will a bit later.
w

witty-crayon-22786

08/07/2020, 12:06 AM
thanks! have a good evening.