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

[skip changelog] chore: logging: switch runtime Print() spots to use the logger #12311

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

ribasushi
Copy link
Collaborator

@ribasushi ribasushi commented Jul 27, 2024

Proposed Changes

While examining a fullsync log noticed that the actor bundle loader prints directly to STDOUT. Grepping briefly through the codebase uncovered only 2 spots like that, so fix them both.

@ribasushi ribasushi changed the title chore: logging: switch runtime Print() spots to use the logger [skip changelog] chore: logging: switch runtime Print() spots to use the logger Jul 27, 2024
@ribasushi
Copy link
Collaborator Author

cc @rvagg for this one I suppose

@ribasushi ribasushi requested a review from rvagg July 29, 2024 04:20
chain/consensus/filcns/upgrades.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Jul 30, 2024

You've made this more controversial than it should be and playing divide and conquer among maintainers is not appreciated.

Please revert the changes entirely in chain/consensus/filcns/upgrades.go and if you really feel strongly enough to bikeshed about a stdout print that's already gated by a tty check then feel free to open a separate PR.

The other change is reasonable.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

remove chain/consensus/filcns/upgrades.go changes

@ribasushi
Copy link
Collaborator Author

ribasushi commented Jul 30, 2024

playing divide and conquer among maintainers is not appreciated.

@rvagg: I hear what you are saying and I call bullshit.

  1. Straightforward, single-commit PR is open during the weekend
  2. You are highlighted to review due to your involvement in chore: ui: add a terminal check #12256 (review)
  3. You request some changes, which I begrudgingly implement without saying anything
  4. @masih and @ZenGround0 step in and say "this is not cool, do the old thing"
  5. I do the old thing, now 2 days later, and am asking y'all as employees of FilOz and holders of all the keys to get your shit together
  6. You write the thing I quoted above

I do not need to tell you how this looks from the outside, for a supposedly-open project to "accept" free, obviously correct contributions like this.

Your mandate is to be stewards, not gatekeepers. Saying "thank you" to contributions is optional. Removing obstructions is not.

in closing: the splash-to-stdout-instead-of-logging-system is still a problem. I am not fixing this one as per request. I hope one of @rvagg @masih @ZenGround0 will.

@ribasushi ribasushi requested a review from rvagg July 30, 2024 09:41
@rvagg
Copy link
Member

rvagg commented Jul 31, 2024

@masih and @ZenGround0 step in and say "this is not cool, do the old thing"

As per @masih:

FWIW, I would vote for removing the cosmetic splash in favour of consistently logging the system behaviour.

I don't read his text or @ZenGround0's 👍 as a vote for your original change but for removing the splash entirely.

That key misunderstanding - either yours or mine - is why I'm asking this to be scaled back because this is not worth anyone's time doing here, for the sake of a splash, that's gated by a tty check and doesn't touch the logs, yet your change makes 36 lines of ascii art go into everyone's logs: https://github.com/filecoin-project/lotus/blob/4cc7061c8f7084a89062b21eafecffea896b1f34/chain/consensus/filcns/UpgradeSplash.txt

@rvagg rvagg merged commit d38ce38 into filecoin-project:master Jul 31, 2024
82 checks passed
@ribasushi ribasushi deleted the chore/less_print branch August 11, 2024 21:56
ribasushi added a commit to ribasushi/ci-abusing-lotus-fork that referenced this pull request Aug 17, 2024
ribasushi added a commit to ribasushi/ci-abusing-lotus-fork that referenced this pull request Aug 20, 2024
ribasushi added a commit to ribasushi/ci-abusing-lotus-fork that referenced this pull request Aug 20, 2024
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.

3 participants