https://pantsbuild.org/ logo
Join Slack
Powered by
<@UB2J9BQA0>, <@U051221NF>: mm. this might be a fu...
# development
w
@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
Oh, both those subtargets got the 
provides=
copied to them?
w
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
Sure, I'll take a look
w
./pants test src/python/pants/backend/python/rules/run_setup_py_test.py
in https://github.com/pantsbuild/pants/pull/10511
thank you!
h
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
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
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
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
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
it is always the case.
h
Great
w
the spec_path is always the location of the target.
h
I do wonder if 
setup-py
should use solely 
UnexpandedTargets
and/or 
addr.maybe_convert_to_base_target()
w
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
so we’d possibly need to expose UnexpandedTransitiveTargets
This would allow us to resolve the concern I have about 
filedeps --transitive --globs
h
I don't see how this is related to compilation?
w
@happy-kitchen-89482: we will need to detect and group by cycles
to create batches of files to compile.
h
OK, so just the concept of grouping. Here cycles aren't the issue.
w
yea. the concept of grouping is going to be important.
h
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
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
Sure, I'm here
w
@hundreds-father-404, @happy-kitchen-89482: pushed… expecting it to go green on this run
🔥 1
❤️ 1
👖 1
h
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
👖 1
🔥 3
❤️ 1
@happy-kitchen-89482: nice!
h
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
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
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
shouldn’t have, no.
h
Oh I see what happened, nm
easy fix
OK all passes
🎉 1
w
woot 😃
h
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
thanks! have a good evening.
Open in Slack
PreviousNext
Powered by