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

bitter-ability-32190

04/10/2023, 7:28 PM
I'm having trouble finding the right seam to insert a deprecation of secondary ownership. Because realistically the only place we use in the core backends is
package
for Python for AWS Lambda and Google Cloud, but it could be used anywhere and where it is resolved is very far removed from where it is used 😐
From `src/python/pants/backend/google_cloud_function/python/target_types.py`'s
filespec
Copy code
if not path.endswith(".py"):
            return {"includes": []}
haha you can't have Python files as Google/AWS cloud handlers unless they end in
.py
womp womp
😮 1
I want to make sure that commands like `package ::`don't warn
w

witty-crayon-22786

04/10/2023, 9:27 PM
hm.
i think that the relevant usage of
OwnersRequest
is in
src/python/pants/engine/internals/specs_rules.py
you could maybe do something like looking at the count of files going in to decide whether it’s a request worth warning for?
but that could still have some false positives i think.
on the other hand: it’s a warning. false positives for the warning is probably ok, since you won’t get false positives once the deprecation is removed.
b

bitter-ability-32190

04/10/2023, 9:33 PM
Yeah no good way. I really wish we could just remove 😑
Turns out
paths
arguments can't be used if it matches secondary fields 🫣
I don't think counts works either. I'm wondering if anyone does --changed-since with
package
.
w

witty-crayon-22786

04/10/2023, 9:48 PM
almost certainly.
b

bitter-ability-32190

04/10/2023, 9:49 PM
We might not be able to remove then. Not without a good workaround
w

witty-crayon-22786

04/10/2023, 9:49 PM
well, that’s what i’m saying about the warning vs error
some false positives in the warning might be ok, since they don’t block anyone.
b

bitter-ability-32190

04/10/2023, 9:50 PM
It's annoying it's only used for package , and for Python, and for only 2 target types. Maybe my paths issue is just an issue with paths, but I get the sense secondary ownership is a bit of an afterthought
Maybe we remove it for run, and make secondary ownership not the default, then goals opt in?
w

witty-crayon-22786

04/10/2023, 9:51 PM
hm. i’m not understanding the
run
vs
package
distinction that you are making.
b

bitter-ability-32190

04/10/2023, 9:52 PM
Well for run, we have to pick one target so we try and disambiguate. Although nothing in our repo has the ambiguity anymore, so just for plugins
For
paths
the args error if you give it a filename that has a secondary owner and the error message literally suggests using the arg provided 🤦‍♂️
Then for all other goals that can work on multiple targets, like `package`theres no problem
w

witty-crayon-22786

04/10/2023, 9:55 PM
For
paths
the args error if you give it a filename that has a secondary owner and the error message literally suggests using the arg provided 🤦‍♂️
we’re no longer talking about the deprecation then? instead, the actual removal of
SecondaryOwnerMixin
?
b

bitter-ability-32190

04/10/2023, 9:55 PM
Yeah that would be ideal. Since it's only used by 2 targets and leads to ambiguities
w

witty-crayon-22786

04/10/2023, 9:56 PM
sure, i’m fine with removing it. just trying to understand what we’re talking about… the deprecation, or the removal.
the deprecation cannot cause errors… it might make some errors more confusing if the warning triggers as a false positive
b

bitter-ability-32190

04/10/2023, 9:57 PM
We can't remove without first warning, so that's what I want to tackle first
w

witty-crayon-22786

04/10/2023, 9:58 PM
ok. then i don’t understand the
paths
case.
is the warning that is triggered there not valid?
b

bitter-ability-32190

04/10/2023, 10:00 PM
No it isn't valid. And I'm just using that as an example of where the current behavior is hazardous
Well maybe valid, but extremely misleading
w

witty-crayon-22786

04/10/2023, 10:02 PM
and by “current behavior”, you mean the “current behavior of the deprecation branch that you have”?
b

bitter-ability-32190

04/10/2023, 10:05 PM
No. Ah yeah that's where we've diverged. There's an error today on mainline pants. If there's a secondary owner of a file,
paths
will error about ambiguity.
And one of the suggestions is the file used in the command (ergo misleading/unhelpful)
w

witty-crayon-22786

04/10/2023, 10:05 PM
sure, fine: i agree that we should deprecate it.
heh
b

bitter-ability-32190

04/10/2023, 10:06 PM
And I bring it up because it shows secondary ownership is hazardous. So either we pull or we go all in
w

witty-crayon-22786

04/10/2023, 10:06 PM
yes, pull, agreed
b

bitter-ability-32190

04/10/2023, 10:06 PM
Hehe so now how to deprecate reasonably well is the question 😂
w

witty-crayon-22786

04/10/2023, 10:07 PM
i think that i gave one answer in my first few posts in the thread
b

bitter-ability-32190

04/10/2023, 10:08 PM
I don't like false positives because the error message is like "something might be removed, do this instead" but there isn't an issue. We scared the user for nothing
I guess we could determine if the returned ownership has only secondary for a filespec. And if so error
Let me try that
Oh wait, that's not possible, because we'll always match primary 🤦‍♂️
Yeah I don't think this is really doable without also modifying the caller's site. E>g.
package