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

Align include guards with OpenOCD coding guidelines. #1103

Conversation

TommyMurphyTM1234
Copy link
Collaborator

Second attempt after I couldn't get this PR to work:

Addresses this issue:

Note that encoding.h has not been changed in this repo as it is programatically generated elsewhere, so the include guard would need to be changed there if possible/appropriate

@TommyMurphyTM1234
Copy link
Collaborator Author

Same issue with checkpatch again - I have no idea what its problem is with the commit message... :-|

Run ./tools/scripts/checkpatch.pl --no-signoff --git FETCH_HEAD..HEAD
WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

total: 0 errors, 1 warnings, 0 checks, 163 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Commit b83598e0c26a ("Align include guards with OpenOCD coding guidelines. Fixes: https://github.com/riscv-collab/riscv-openocd/issues/109[7](https://github.com/riscv-collab/riscv-openocd/actions/runs/9836798453/job/27153270607?pr=1103#step:5:8)") has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BLOCK_COMMENT_STYLE COMPLEX_MACRO CONST_STRUCT ENOSYS FILE_PATH_CHANGES GERRIT_CHANGE_ID LINE_SPACING LOGICAL_CONTINUATIONS MACRO_WITH_FLOW_CONTROL NEW_TYPEDEFS PARENTHESIS_ALIGNMENT PREFER_DEFINED_ATTRIBUTE_MACRO PREFER_FALLTHROUGH PREFER_KERNEL_TYPES SPLIT_STRING SSCANF_TO_KSTRTO SWITCH_CASE_INDENT_LEVEL TRACING_LOGGING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the openocd-devel mailing list or prepare a patch
      and send it to Gerrit for review.
Error: Process completed with exit code 1.

@TommyMurphyTM1234
Copy link
Collaborator Author

Maybe it has a problem with this:

Commit b83598e0c26a ("Align include guards with OpenOCD coding guidelines. Fixes: https://github.com/riscv-collab/riscv-openocd/issues/109[7](https://github.com/riscv-collab/riscv-openocd/actions/runs/9836798453/job/27153270607?pr=1103#step:5:8)") has style problems, please review.

But I have no idea where this text came from because I never entered it as part of the commit message:

[7](https://github.com/riscv-collab/riscv-openocd/actions/runs/9836798453/job/27153270607?pr=1103#step:5:8)

@TommyMurphyTM1234
Copy link
Collaborator Author

If anybody can advise on how to rectify this - ideally by amending the original commit message and then getting that pushed to my forked repo and then reflected in this PR so that I don't have to do everything again from scratch - please let me know. Thanks.

@TommyMurphyTM1234
Copy link
Collaborator Author

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

I tried this:

./tools/scripts/checkpatch.pl --fix

but it never returned and I had to kill it.

@JanMatCodasip
Copy link
Collaborator

JanMatCodasip commented Jul 8, 2024

WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

My guess would be that checkpatch wants to see commit message in this form:

Commit title as a single line
<blank line>
Single- or multi-line commit description

Other than this detail, the changes look good.

@JanMatCodasip
Copy link
Collaborator

If anybody can advise on how to rectify this - ideally by amending the original commit message and then getting that pushed to my forked repo and then reflected in this PR

I hope it is enough to:

git commit --amend  # and update the message to make checkpatch happy
git push origin HEAD -f

@en-sc
Copy link
Collaborator

en-sc commented Jul 8, 2024

@TommyMurphyTM1234, the issue with your commit message is that you have missed a blank line between the first sentence and the second one.
See https://git-scm.com/docs/git-commit#_discussion on how commit message is split into commit title and commit description.
The PR to your repo: https://github.com/TommyMurphyTM1234/riscv-openocd/pull/1

@en-sc
Copy link
Collaborator

en-sc commented Jul 8, 2024

WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

My guess would be that checkpatch wants to see commit message in this form:

Commit title as a single line
<blank line>
Single- or multi-line commit description

Other than this detail, the changes look good.

You are right. I've missed this while writing my previous comment.

@TommyMurphyTM1234
Copy link
Collaborator Author

If anybody can advise on how to rectify this - ideally by amending the original commit message and then getting that pushed to my forked repo and then reflected in this PR

I hope it is enough to:

git commit --amend  # and update the message to make checkpatch happy
git push origin HEAD -f

Thanks @JanMatCodasip (and @en-sc) for the comments.

I used the commands above to change the commit message to add a blank line between the two existing lines and then do a force push and it looks like checkpatch is happy now. :-)

src/target/riscv/debug_defines.h Outdated Show resolved Hide resolved
@TommyMurphyTM1234
Copy link
Collaborator Author

TommyMurphyTM1234 commented Jul 8, 2024

I give up - I can't deal with checkpatch and its constant objections.

Now it's saying:

WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1
#3172: FILE: src/target/riscv/debug_defines.h:1:

Even though I reverted back to the original version of this file which has no such license id line.

@en-sc
Copy link
Collaborator

en-sc commented Jul 8, 2024

@TommyMurphyTM1234, please, try squashing the commits by running:

> git rebase -i HEAD~3
  • This will open your editor, prompting you to specify how exactly to rebase your commits.
  • Now you should change all but the first 'pick's to 'squash' and exit the editor
  • The editor will re-open, prompting you to modify the commit for the new squashed commit.
  • Leaving the message from the first commit should work just fine.

@en-sc
Copy link
Collaborator

en-sc commented Jul 8, 2024

You are getting all these errors due to #1085.
#1085 changed the way Checkpatch is ran.
Before #1085 it was ran on the diff.
After #1085 it is ran on the commits -- the diff + the messages.
I've made the change for two reasons:

  • This is closer to how it's done in the mainline OpenOCD.
  • This makes it possible to add Checkpatch-ignore: directives to the commit message, so that Checkpatch will ignore some warnings.

@TommyMurphyTM1234
Copy link
Collaborator Author

You are getting all these errors due to #1085. #1085 changed the way Checkpatch is ran. Before #1085 it was ran on the diff. After #1085 it is ran on the commits -- the diff + the messages. I've made the change for two reasons:

  • This is closer to how it's done in the mainline OpenOCD.
  • This makes it possible to add Checkpatch-ignore: directives to the commit message, so that Checkpatch will ignore some warnings.

Thanks for the comments/suggestions and explanations @en-sc. I find checkpatch very confusing but your comments have helped to clarify some issues. I'll attend to the changes tomorrow and hopefully get this PR pushed on. Thanks again.

@TommyMurphyTM1234
Copy link
Collaborator Author

@TommyMurphyTM1234, please, try squashing the commits by running:

> git rebase -i HEAD~3
  • This will open your editor, prompting you to specify how exactly to rebase your commits.
  • Now you should change all but the first 'pick's to 'squash' and exit the editor
  • The editor will re-open, prompting you to modify the commit for the new squashed commit.
  • Leaving the message from the first commit should work just fine.

Hi @en-sc, I did this.
I presume that I then need to push the changes?
But I tried that and got this:

git push
To github.com:TommyMurphyTM1234/riscv-openocd.git
 ! [rejected]            fix-include-guards -> fix-include-guards (non-fast-forward)
error: failed to push some refs to 'github.com:TommyMurphyTM1234/riscv-openocd.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

If I do git pull I get this:

git pull
hint: You have divergent branches and need to specify how to reconcile them.
hint: You can do so by running one of the following commands sometime before
hint: your next pull:
hint:
hint:   git config pull.rebase false  # merge (the default strategy)
hint:   git config pull.rebase true   # rebase
hint:   git config pull.ff only       # fast-forward only
hint:
hint: You can replace "git config" with "git config --global" to set a default
hint: preference for all repositories. You can also pass --rebase, --no-rebase,
hint: or --ff-only on the command line to override the configured default per
hint: invocation.
fatal: Need to specify how to reconcile divergent branches.

Would it be simpler to just try from scratch a third time?

@en-sc
Copy link
Collaborator

en-sc commented Jul 9, 2024

@TommyMurphyTM1234, git push -f should work

@TommyMurphyTM1234
Copy link
Collaborator Author

@TommyMurphyTM1234, git push -f should work

Thanks - that allowed me to push but now checkpatch is failing yet again on some other issue.

Run ./tools/scripts/checkpatch.pl --no-signoff --git FETCH_HEAD..HEAD
  ./tools/scripts/checkpatch.pl --no-signoff --git FETCH_HEAD..HEAD
  shell: /usr/bin/bash -e {0}
  env:
    DL_DIR: ../downloads
    BUILD_DIR: ../build
ERROR:EXECUTE_PERMISSIONS: do not set execute permissions for source files
#66: FILE: src/target/riscv/debug_defines.h

WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1
#71: FILE: src/target/riscv/debug_defines.h:1:
+

WARNING:MISSING_EOF_NEWLINE: adding a line without newline at end of file
#80: FILE: src/target/riscv/debug_defines.h:3157:
+#endif

total: 1 errors, [2](https://github.com/riscv-collab/riscv-openocd/actions/runs/9854803162/job/27208350939?pr=1103#step:5:2) warnings, 0 checks, 156 lines checked

@TommyMurphyTM1234
Copy link
Collaborator Author

Closing this PR and I will try again with from scratch.

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