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

Implement debugPrint for built-in file providers #24156

Closed
wants to merge 3 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Oct 31, 2024

Simplifies print style debugging.

@fmeum fmeum requested a review from a team as a code owner October 31, 2024 10:51
@fmeum fmeum requested review from gregestren and comius and removed request for a team and gregestren October 31, 2024 10:51
@github-actions github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Oct 31, 2024
Copy link
Contributor

@comius comius left a comment

Choose a reason for hiding this comment

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

This is only for print, right? Not for str which would be a bigger change.

I'd go for a more Starlark representation, printing those providers as if they were implemented in starlark.

@comius
Copy link
Contributor

comius commented Dec 9, 2024

cc @brandjon @tetromino @lberki
You might have an opinion about this as well...

@brandjon
Copy link
Member

brandjon commented Dec 9, 2024

If it doesn't expose non-hermetic / non-deterministic information that we don't want Starlark users to introspect on, then it should be safe for str()/repr() and not just print().

But limiting it to print() is less of a breaking change, if that's what you're worried about. Still, I think a str() breakage in a provider isn't that big a deal.

@fmeum fmeum force-pushed the print-runfiles branch 2 times, most recently from 66b8bef to da35a8b Compare December 14, 2024 15:28
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 14, 2024

I'd go for a more Starlark representation, printing those providers as if they were implemented in starlark.

That's a good point. I've modified the methods to use struct syntax, including alphabetic ordering of keys. A positive side effect of this is that you can use buildifier to prettify the output if you want to.

I noticed that the providers that derive from StructImpl don't print their type (e.g. OutputGroupInfo is just a struct(...)). This has some benefits in terms of consistency, but it also loses information. What would you prefer?

@fmeum fmeum requested a review from comius December 14, 2024 15:29
@comius
Copy link
Contributor

comius commented Jan 24, 2025

That's a good point. I've modified the methods to use struct syntax, including alphabetic ordering of keys. A positive side effect of this is that you can use buildifier to prettify the output if you want to.

Nice.

I noticed that the providers that derive from StructImpl don't print their type (e.g. OutputGroupInfo is just a struct(...)). This has some benefits in terms of consistency, but it also loses information. What would you prefer?

My preference would be to print out the Name in all cases (either native or Starlark). I don't know what's current state.
I'm leaning more to Name, because it matters, i.e. OutputGroupInfo(a=["x"]) != struct(a =["x"])

@fmeum fmeum removed the request for review from a team January 29, 2025 07:57
@fmeum fmeum requested a review from comius January 29, 2025 08:00
@comius comius added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jan 31, 2025
@fmeum
Copy link
Collaborator Author

fmeum commented Jan 31, 2025

@bazel-io fork 8.1.0

@lberki
Copy link
Contributor

lberki commented Jan 31, 2025

(late to the party)

Yes, I do have an opinion, which is that this is a good thing :)

@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jan 31, 2025
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Jan 31, 2025
Simplifies `print` style debugging.

Closes bazelbuild#24156.

PiperOrigin-RevId: 721850583
Change-Id: I8b3c3a1063f1dbfa3c0c1556e1112c1885a5c6a9
github-merge-queue bot pushed a commit that referenced this pull request Feb 3, 2025
Simplifies `print` style debugging.

Closes #24156.

PiperOrigin-RevId: 721850583
Change-Id: I8b3c3a1063f1dbfa3c0c1556e1112c1885a5c6a9

Commit
fe7b4ab

Co-authored-by: Fabian Meumertzheim <[email protected]>
@fmeum fmeum deleted the print-runfiles branch February 3, 2025 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants