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

libzfs: write_inuse_diffs_one: format strerror() with "%s" #12197

Merged
merged 1 commit into from
Jun 4, 2021

Conversation

nabijaczleweli
Copy link
Contributor

@nabijaczleweli nabijaczleweli commented Jun 4, 2021

Motivation and Context

Introduced in 50353db, breaks CI for every child: https://github.com/nabijaczleweli/zfs/runs/2749781519?check_suite_focus=true#step:6:1425

Description

"%s"

How Has This Been Tested?

No longer makes the warning for me locally

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the OpenZFS code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes. – none apply
  • I have run the ZFS Test Suite with this change applied. – if it builds, it builds
  • All commit messages are properly formatted and contain Signed-off-by.

@nabijaczleweli
Copy link
Contributor Author

nabijaczleweli commented Jun 4, 2021

@tonynguien you merged #12072, could I convince you to fast-track this? CI refusing to build literally anything based on master is gonna be Mildly Annoying

@rincebrain
Copy link
Contributor

rincebrain commented Jun 4, 2021

That's odd, I wonder why it passed all the CI before being pulled.

My apologies for the inconvenience.

edit: Yeah, this is somewhat odd, nothing is jumping out at me as different between the two runs...

Fixes: 50353db ("Let zfs diff be more
 permissive")
Ref: https://github.com/nabijaczleweli/zfs/runs/2749781519?check_suite_focus=true#step:6:1425
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
@nabijaczleweli
Copy link
Contributor Author

nabijaczleweli commented Jun 4, 2021

Looking at the source branch for the PR, it looks like it was based on master as of 15 days ago, but 739cfb9 (which marked the printf-likes as printf-likes) only landed 2 days ago.
That being said, I'd expect at least the GHA jobs to work on the merge and not the raw source? (But then I may be expecting a basic well-understood and widely-implemented feature from GHA, which has been shown to be a mistake. Either way, you're at no fault, CI shoulda caught this.)

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Thanks for catching this right away, let me verify it locally then get it merged to keep the CI happy.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Jun 4, 2021
@rincebrain
Copy link
Contributor

I wonder if Github has any way to express the idea of checking whether a PR is based against a sufficiently recent version of the branch (beyond just "yup it merges cleanly") - I don't see an obvious one. Maybe if you can trigger a check nightly on all open PRs, it would be good enough? (Or I guess you could trigger it on every push to the main branch if it's cheap enough...)

Just a thought.

Copy link
Contributor

@tonynguien tonynguien left a comment

Choose a reason for hiding this comment

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

Thanks for jumping on this quickly.

@behlendorf
Copy link
Contributor

behlendorf commented Jun 4, 2021

@rincebrain there is a GitHub feature for this called "Require branches to be up to date before merging" which does close to what we want. But my understanding it it's very strict, so PRs would always need to be against the HEAD commit. Which clearly isn't ideal when there are a large number of PRs open. But we could always give it a try and see if it really does behave that way.

@rincebrain
Copy link
Contributor

@rincebrain there is a GitHub feature for this called "Require branches to be up to date before merging" which does close to what we want. But my understanding it it's very strict, so PRs would always need to be against the HEAD commit. Which clearly isn't ideal when there are a large number of PRs open. But we could always give it a try and see if it really does behave that way.

Oof, the behavior as described seems like a nightmare for a project with any nontrivial flow of PRs.

Maybe I'll go try learning GH actions on a toy project and see how easily it can be made to do what I described...

@behlendorf
Copy link
Contributor

Thankfully we don't run in to things like this that often since normally the PRs are pretty current, and things don't change that fast. It's just that lately @nabijaczleweli has been raising the bar for many of the automated checks (which is great!).

@behlendorf behlendorf self-assigned this Jun 4, 2021
@behlendorf behlendorf merged commit e5e76bd into openzfs:master Jun 4, 2021
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 8, 2021
Fixes 50353db ("Let zfs diff be more  permissive") which accidentally
introduced a build warning.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12197
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 8, 2021
Fixes 50353db ("Let zfs diff be more  permissive") which accidentally
introduced a build warning.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12197
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 9, 2021
Fixes 50353db ("Let zfs diff be more  permissive") which accidentally
introduced a build warning.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12197
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants