Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix secondary ownership warning semantics #19191

Merged
merged 8 commits into from
May 31, 2023
Merged

Fix secondary ownership warning semantics #19191

merged 8 commits into from
May 31, 2023

Conversation

thejcannon
Copy link
Member

@thejcannon thejcannon commented May 30, 2023 •
edited
Loading

Fixes #19174 by boiling the criteria to warn down to:

  1. The Address/Target/FieldSet is matched due only to SecondaryOwnerMixin
  2. It was matched due to a file spec (literal or glob)
  3. It wasn't matched due ton address spec
    • This is just to handle the corner case where it was matched due to file and address

In order to facilitate 1., Owner and find_owner have been (temporarily) modified. Additionally, the rule that goes from RawSpecsWithOnlyFileOwners -> Addresses has been split to return Owners, with an additional rule added to complete the graph (Owners -> Addresses).

Then 2 and 3 are facilitated by just requesting the addresses based on the include specs.

@thejcannon thejcannon added needs-cherrypick category:internal CI, fixes for not-yet-released features, etc. labels May 30, 2023
@thejcannon thejcannon added this to the 2.17.x milestone May 30, 2023
@thejcannon
Copy link
Member Author

...I never was particularly great at algorithms.

@huonw
Copy link
Contributor

huonw commented May 30, 2023

Thanks for working on this. I tried it on the reproducer from #19174, by:

  1. checking this branch out
  2. saving the script in Unactionable(?) "indirectly referring to a target" warning for pants package :: with pex or FaaS #19174 to /tmp/script.sh
  3. running MODE=debug PANTS_SOURCE=~/projects/pantsbuild/pants bash /tmp/script.sh

I still got the warning, unfortunately.

@huonw
Copy link
Contributor

huonw commented May 30, 2023

I haven't understood the code deeply yet, but this code seems like it's trying to reverse engineer a decision by some lower layer (maybe something about Owners/OwnersRequest/find_owners?), about selecting a target via a SecondaryOwnerMixin field. Is there a way to either:

  • push the warning down to that layer, or
  • record information there about why a target/field set was selected, and then this higher level can reference that?

@thejcannon
Copy link
Member Author

The idea was to be surgical since it'll be removed shortly, so muddying the happy path code temporarily seemed annoying.

@thejcannon
Copy link
Member Author

Yeah looks like a very obvious bug in the code 🤦

@thejcannon
Copy link
Member Author

record information there about why a target/field set was selected, and then this higher level can reference that?

Ah yeah, OK this seems to be the only way to really get this right.

stuhood
stuhood approved these changes May 31, 2023
View reviewed changes
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

thejcannon
thejcannon commented May 31, 2023
View reviewed changes
src/python/pants/engine/internals/graph.py
@@ -986,16 +996,17 @@ def create_live_and_deleted_gets(
)
for secondary_owner_field in secondary_owner_fields:
matching_files.update(
*secondary_owner_field.filespec_matcher.matches(list(sources_set))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bugfix. See https://pantsbuild.slack.com/archives/C0D7TNJHL/p1685550973185069

@thejcannon thejcannon merged commit 71c6fe6 into main May 31, 2023
@thejcannon thejcannon deleted the jcannon/fix19174 branch May 31, 2023 19:27
@thejcannon thejcannon added the auto-cherry-picking-failed Auto Cherry-Picking Failed label Jun 1, 2023
thejcannon added a commit that referenced this pull request Jun 1, 2023
@thejcannon
Fix secondary ownership warning semantics (#19191)
5aff794 
Fixes #19174 by boiling the criteria to warn down to:

1. The Address/Target/FieldSet is matched due __only__ to
`SecondaryOwnerMixin`
2. It was matched due to a file spec (literal or glob)
3. It wasn't matched due ton address spec
- This is just to handle the corner case where it was matched due to
file and address

In order to facilitate 1., `Owner` and `find_owner` have been
(temporarily) modified. Additionally, the rule that goes from
`RawSpecsWithOnlyFileOwners` -> `Addresses` has been split to return
`Owners`, with an additional rule added to complete the graph (`Owners`
-> `Addresses`).

Then 2 and 3 are facilitated by just requesting the addresses based on
the include specs.
thejcannon added a commit that referenced this pull request Jun 2, 2023
@thejcannon
Fix secondary ownership warning semantics (Cherry-pick of #19191) (#1…
ed3ddac 
…9224)

Fixes #19174 by boiling the criteria to warn down to:

1. The Address/Target/FieldSet is matched due __only__ to
`SecondaryOwnerMixin`
2. It was matched due to a file spec (literal or glob)
3. It wasn't matched due ton address spec
- This is just to handle the corner case where it was matched due to
file and address

In order to facilitate 1., `Owner` and `find_owner` have been
(temporarily) modified. Additionally, the rule that goes from
`RawSpecsWithOnlyFileOwners` -> `Addresses` has been split to return
`Owners`, with an additional rule added to complete the graph (`Owners`
-> `Addresses`).

Then 2 and 3 are facilitated by just requesting the addresses based on
the include specs.
@wisechengyi wisechengyi mentioned this pull request Jun 3, 2023
Merged
wisechengyi added a commit that referenced this pull request Jun 4, 2023
@wisechengyi
Prep 1.28.0.dev1 (#19233)
46b9518 
### Internal

* upgrade to Rust v1.70.0
([#19228](#19228))

* Remove the last mentions of NO_TOOL_LOCKFILE.
([#19229](#19229))

* Upgrade Helm unittest
([#19220](#19220))

* Prepare `2.17.0rc0`.
([#19226](#19226))

* Use a trait for remote action result caching
([#19097](#19097))

* Port `Field` to Rust
([#19143](#19143))

* Upgrade `pyo3` to `0.19`.
([#19223](#19223))

* Prepare `2.16.0rc5`.
([#19221](#19221))

* internal: Create dep inference request type
([#19001](#19001))

* Avoid requiring Python when trying to install Python (using Pyenv)
([#19208](#19208))

* Fix secondary ownership warning semantics
([#19191](#19191))

* Bump os_pipe from 1.1.3 to 1.1.4 in /src/rust/engine
([#19202](#19202))

* Include committer date in local version identifier of unstable builds
([#19179](#19179))

* Ensure we set the AWS region.
([#19178](#19178))
@benjyw benjyw mentioned this pull request Jun 8, 2023
Merged
@huonw huonw mentioned this pull request Jun 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuhood stuhood stuhood approved these changes

@huonw huonw Awaiting requested review from huonw

@Eric-Arellano Eric-Arellano Awaiting requested review from Eric-Arellano

Assignees
No one assigned
Labels
auto-cherry-picking-failed Auto Cherry-Picking Failed category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Milestone
2.17.x
Development

Successfully merging this pull request may close these issues.

Unactionable(?) "indirectly referring to a target" warning for pants package :: with pex or FaaS
3 participants
@thejcannon @huonw @stuhood