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.

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

Remove the rationale field from BashBinaryRequest. #12624

Merged
merged 1 commit into from Aug 23, 2021

Conversation

benjyw
Copy link
Sponsor Contributor

@benjyw benjyw commented Aug 23, 2021

It exists only to provide slightly more context for an error
message. But this context isn't actionable (the rationale doesn't
affect how you fix the error). And the downside is that the
rationale affects the cache key, so we have to re-find bash
for every rationale, which is superfluous extra work.

Right now there are only two rationales, so the work is only
done twice instead of once (and the underlying process is still cached
so the extra work is just in-process). But this field models "bad behavior":
If in the future someone, naively, makes the rationale a dynamic
message, here or in some similar case that they cargo-culted from here,
then the rule will thrash. Better to remove this landmine preemptively.

[ci skip-rust]

[ci skip-build-wheels]

It exists only to provide slightly more context for an error
message. But this context isn't actionable (the rationale doesn't
affect how you fix the error), and the downside is that the
rationale affects the cache key, so we have to re-find bash
for every rationale, which is superfluous extra work.

If in the future someone, naively, makes the rationale a dynamic
message, then we will thrash for unclear reasons. Better to remove
this landmine preemptively.

[ci skip-rust]

[ci skip-build-wheels]
@benjyw benjyw merged commit b8b3e50 into pantsbuild:main Aug 23, 2021
@benjyw benjyw deleted the no_rationale branch August 23, 2021 16:10
@@ -477,7 +477,6 @@ class BashBinary(BinaryPath):

@dataclass(frozen=True)
class BashBinaryRequest:
rationale: str
Copy link
Member

Choose a reason for hiding this comment

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

Since the rationale provided little value in this case, the change probably makes sense. That said, if we expected more fluent use of dataclasses / more thought about the types used in rule signatures / Gets, this could just have been, IIUC:

@dataclass(frozen=True)
class BashBinaryRequest:
    rationale: str = dataclasses.field(compare=False)

@stuhood
Copy link
Sponsor Member

stuhood commented Aug 23, 2021

The Process.description field is ignored in equality for the underlying intrinsics: see

let a = process_generator("One thing".to_string(), Some(Duration::new(0, 0)));
let b = process_generator("Another".to_string(), Some(Duration::new(0, 0)));
let c = process_generator("One thing".to_string(), Some(Duration::new(5, 0)));
let d = process_generator("One thing".to_string(), None);
// Process should derive a PartialEq and Hash that ignores the description
assert_eq!(a, b);
assert_eq!(hash(&a), hash(&b));

How did you confirm that the process was running multiple times?

@benjyw
Copy link
Sponsor Contributor Author

benjyw commented Aug 23, 2021

I didn't confirm it. In fact I mentioned the opposite - that it wouldn't run multiple times. But that's mainly because we were only using the rationale for the error message, not for the process description.

This is more about not superfluously re-running and re-caching rules that differ only on this rationale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants