bootstrap: PathSet::check only considers starts_with for --skip flag

[jyn514]

The original motivation for adding this path.starts_with check was to allow things like --skip=src/etc: #133492. But it affects quite a lot more things than just --skip; bootstrap is really not designed for the case when multiple Steps match the same filter (#134919 (comment)). For example, x test src does ... something! Not sure what, but something!

This has already caused several issues; see #135022, #135022, probably others I am forgetting.

Change starts_with to only affect --skip, not anything else.

The immediate motivation for this change was to prevent x doc --open src/doc from opening a dozen different pages (see was_explicitly_invoked for how this is related).

r? bootstrap cc @marcoieni @onur-ozkan @jieyouxu @Zalathar

[jyn514]

the failure is because the path("compiler/rustc_codegen_*") in all the codegen backends is now being ignored. that seems ok, actually? I wouldn't expect x test --stage 2 compiler to test codegen backends given the current should_run.

I've added a manual .path("compiler") to each of the backends, restoring the old behavior as a one-off for those specific steps.

As an aside, I'm not sure the snapshot test is testing what it intends to ... I don't think it realizes that the stage 2 compiler is only built as a dependency of the codegen backends.

[Zalathar]

I'm very wary of seeing cryptic booleans threaded through functions that are already have mysterious semantics to begin with.

This seems like it's going to cause a lot of frustration for whoever tries to properly clean up PathSet someday.

[jyn514]

Would you like me to add more doc-comments, or convert it to an enum? I really do think the current behavior is not correct ... Happy to hear other suggestions for fixing it.

[Kobzol]

The path checking in bootstrap is quite.. interesting 😆 Tbh I don't think it's unreasonable to expect that x test compiler runs tests of everything under the compiler directory, not just compiler tests. The issue is that we use aliases and actual on-disk paths kind of interchangeably, but sometimes there is a meaningful difference.

Ideally, we would revamp the whole system from the ground up based on some agreed-upon reasoning that is well-defined for all our current use-cases, but.. you know 😆

I think that at some places we should be using prefixes, at some places suffixes/prefixes, at some places an exact match, etc., but that's not trivial to do at the moment. For your fix, I would appreciate using at least an enum (as Zalathar mentioned), so that we can make it a bit more explicit how does the matching work.

As an aside, I'm not sure the snapshot test is testing what it intends to ... I don't think it realizes that the stage 2 compiler is only built as a dependency of the codegen backends.

The snapshot tests simply print all non-trivial steps that are executed for a given bootstrap invocation, they don't care whether they are built as a dependency or not. We want to see e.g. if some step is suddenly becoming executed even if only as a dependency.

[jieyouxu]

I think that at some places we should be using prefixes, at some places suffixes/prefixes, at some places an exact match, etc., but that's not trivial to do at the moment. For your fix, I would appreciate using at least an enum (as Zalathar mentioned), so that we can make it a bit more explicit how does the matching work.

I have a concern about spot-fixing the pathset-related handling logic, this is an aspect that I genuinely think we as a team need to take a step back, and survey all the different variations of pathset-like handling we have in bootstrap, before changing its current behavior any further. The pathset-related handling, at least when I briefly had a look, was the product of organic growth over the years, and there's no overarching design philosophy (even if there was, the PathSet abstraction is broken by many other variations of path filter handling).

I understand that this PR only

Change starts_with to only affect --skip, not anything else.

But I really would encourage us to take a step back and have a closer look at the larger picture surrounding pathset logic before making any changes to its current behavior.

[jieyouxu]

That is to say, usually I would be fine with these kind of fixes, but the PathSet-related logic is so much of a mess that I can almost confidentally say that nobody fully understands it that I rather block this fix on first figuring out what our design for the path filtering -> step dispatching/matching behavior is.

I fully agree that the current behavior surely isn't right, and "what should be considered correct" is the question that I would like the team to discuss.

EDIT: cf. #t-infra/bootstrap > Path filters in bootstrap