Skip to content

ci: Print iwyu patch in git diff format #27012

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.

Sign up for GitHub

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

Jump to bottom
Merged
fanquake merged 1 commit into from
Feb 2, 2023
Merged

ci: Print iwyu patch in git diff format #27012

fanquake merged 1 commit into from
Feb 2, 2023

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Feb 1, 2023

Seems more dev friendly to also have a patch to copy-paste

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 1, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK hebasto, fanquake, stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26763 (ci: Treat IWYU violations as errors by hebasto)
  • #25797 (build: Add CMake-based build system by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot added the Tests label Feb 1, 2023
stickies-v
stickies-v reviewed Feb 1, 2023
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

Concept ACK

@hebasto
Copy link
Member

hebasto commented Feb 1, 2023

Concept ACK. Thank you for picking it up!

#26763 (comment):

It is possible to leverage the fix_includes.py tool

Could probably make sense to add this to the CI, to allow easy copy-pasting from Cirrus or from a local run.

@maflcko maflcko force-pushed the 2302-ci-iwyu-git-diff- branch from fa26904 to fa6986a Compare February 1, 2023 13:12
@hebasto
Copy link
Member

hebasto commented Feb 1, 2023 •
edited
Loading

a patch to copy-paste

To be pedantic, a manual replacement quotes "..." with angle brackets <...> is still required for non-system headers, unfortunately.

hebasto
hebasto approved these changes Feb 1, 2023
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK fa6986a, tested on Ubuntu 22.04 locally.

@maflcko
Copy link
Member Author

maflcko commented Feb 1, 2023

Example output: https://cirrus-ci.com/task/5039323280572416?logs=ci#L5652

This was referenced Feb 1, 2023
Closed
Closed
fanquake
fanquake approved these changes Feb 2, 2023
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK fa6986a - did not test but example CI output looks ok.

stickies-v
stickies-v approved these changes Feb 2, 2023
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

utACK fa6986a

@fanquake fanquake merged commit c2028f9 into bitcoin:master Feb 2, 2023
@maflcko maflcko deleted the 2302-ci-iwyu-git-diff-📖 branch February 3, 2023 13:19
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 3, 2023
@fanquake
Merge bitcoin#27012: ci: Print iwyu patch in git diff format
a2a2f91
@bitcoin bitcoin locked and limited conversation to collaborators Feb 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@fanquake fanquake fanquake approved these changes

@hebasto hebasto hebasto approved these changes

+1 more reviewer

@stickies-v stickies-v stickies-v approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @DrahtBot @hebasto @fanquake @stickies-v