Add a sample of git pre-commit hook

[yamt]

No description provided.

[xiaoxiang781216]

Call tools/checkpatch.sh intead, the benefit include:
1.Generate nxstyle automatically
2.Get more precheck in the furture
a.copyright check
b.spell check
3.The same result as github action(it call checkpatch.sh too)

[yamt]

i was not aware of checkpatch.sh.
personally i'm not comfortable to run make in git hooks.

[xiaoxiang781216]

But it is helping newbie to setup environment, it's hard to generate nxstyle from command line manually. Anyway, we can disscuss whether checkpatch.sh should or not invoke make automatically, but it's better to call checkpatch.sh here to ensure the consistent result with precheck process.

[xiaoxiang781216]

BTW, I am considering that is it better that we add one option in checkpatch.sh to generate pre-commit and put it into .git/hook?

[yamt]

i'd like to suggest checkpatch.sh minimum. do not install tools, or set up git hooks.
instead, introduce a separate script to do those "heavy" jobs.
how do you think?

[xiaoxiang781216]

@yamt you can run checkpatch.sh with -r option to ensure your modification don't introduce the new coding style problem.
It's better to give your experience here:
https://lists.apache.org/thread.html/r6e7b2ed2d5ca1d5b49c5898b591182a8d7475bee2d5de344c026b3f5%40<dev.nuttx.apache.org>

[davids5]

@davids5 as I said before there isn't real difference between checkpatch.sh and make check_format, and it's very easy to implement check_format on top of checkpatch.sh and the patch is welcome. We talk before the reason to select checkpatch.sh as the base and let pre-commit/check_format to inovke checkpatch.sh, let me reemphasis again here:
1.The bash script can be invoke easily from most environment(jenkins, travis, github action, git hook...).
2.The coding style isn't the only check we need to do, we need do more:
a.spell check
b.copyright check
c.defconfig check
If we call nxstyle directly from pre-commit/check_format/checkpatch/..., could you tell me how can we improve(most likely) the precheck flow in the furture?
3.Since we can pass the argument to a bash script, it's very easy to let checkpatch.sh support many different usecase(patch file, source file, commit id, or even stdin). Could you tell me how can you do this in Makefile?
4.Since nxstyle isn't perfect, I have saw many discussion(at least three times) in email list to replace nxstyle with clang-format, uncrustify and more. We can adapter this change quickly if all place invoke checkpath.sh instead of nxstyle.

I am not arguing against having the script. I never have.

1. Just do not make it the the swiss arm knife. Break this stuff out to files that have names that make sense. (checkpatch should check a patch, or call it checkcontrib [ution] if it does it all things, spellcheck should check spelling...) Do this to reduce coupling. If you have common code include it. Have a separate install script for the hooks.

2. Add simple targets to make' - this will simplify the docs and user experiences and add an abstraction. Thy all can run the script. Do as much for the users as you can - this will empower them and make more contributors.

make check_format
make check_license
make check_spelling
make check_....

3. Solve the issue running the correct version of the nxstlye without the uses needed to deal with the mess. If you feel submodules are to difficult. wget the file from githup from master, add it to the ignore and build it. We just need this to not be a repeated point of error. It waste peoples time over and over again.

[xiaoxiang781216]

1. Just do not make it the the swiss arm knife. Break this stuff out to files that have names that make sense. (checkpatch should check a patch, or call it checkcontrib [ution] if it does it all things, spellcheck should check spelling...) Do this to reduce coupling. If you have common code include it. Have a separate install script for the hooks.

The goal of checkpatch.sh is to ensure the patch is good to merge, so it make sense to check license/spell in checkpatch.sh too. You can implement license/spelling in another script/program just like nxstyle, but user just need run checkpatch.sh to know whether his/her patch is ready for merging.
1.Do you want user invoke check_format/check_license/check_spelling for every PR?
2.Do you want to update all pre-commit/Jenkins/github action scripts if you need add a new precheck?
Or all callers(user/pre-commit/jenkin/makefile/github) invoke one script and we just modify this one script to adapter the new workflow requirement. Please remember we may change the precheck frequently before we lockdown the workflow.

2. Add simple targets to make' - this will simplify the docs and user experiences and add an abstraction. Thy all can run the script. Do as much for the users as you can - this will empower them and make more contributors.

make check_format
make check_license
make check_spelling
make check_....

It's better to just has one target(check_patch), so the user don't need to invoke make several time to know his/her patch is good. Or let user invoke check_format today, check_format/check_license tomorrow etc.

1. Solve the issue running the correct version of the nxstlye without the uses needed to deal with the mess. If you feel submodules are to difficult. wget the file from githup from master, add it to the ignore and build it. We just need this to not be a repeated point of error. It waste peoples time over and over again.

The correct vesion is always the latest one on the master, we need rebase/cherry-pick our patch with the master anyway, why don't we run checkpatch.sh before sending PR?

[davids5]

@xiaoxiang781216 Make can run all the pieces. Please do not only think abut this ONLY the way you have to work. Good tools solve everyone problems are self documenting. What if a user is doing a style cleanup NOW and not concerned about about licenses in the PR?

The documentation of the script tools is poor. There are no examples of what the arguments take as values. Forcing a user to decodes the script when it is not necessary is not inviting. Why is it it tools/configure.sh imx-1060evk/nsh not `make imx-1060evk/nsh'?

A lot of pieces all over the place is fine jut keep them out the the users face and have reasonable granularity.

Yes, we can add more small step like spell/copyright, but it's better that:
1.Implment these check as the bash script or program
2.Invoke them from Makefile/checkpatch.sh/pre-commit
3.check_patch should be the target invoked in normal case, so we can improve the action as need with the furture workflow. Other targets can be used in the special case.

The correct version is always the latest one on the master, we need rebase/cherry-pick our patch with the master anyway, why don't we run checkpatch.sh before sending PR?

No we do not. I explained this already (in email and comments) that work flow is a waste of time and effort with back and forth chery-picking. You do not have a good solution.

[xiaoxiang781216]

@xiaoxiang781216 Make can run all the pieces. Please do not only think abut this ONLY the way you have to work. Good tools solve everyone problems are self documenting.

I already suggest you provide a patch to add check_format(actually check_patch may a better name) in Makefile, what I oppose is to call nxstyle directly in Makefile. It's better to call checkpatch.sh so devloeper can get the same result in his machine as the github CI.

What if a user is doing a style cleanup NOW and not concerned about about licenses in the PR?

Do you monitor the recent comment or PR from @patacongo?
#414
https://github.com/apache/incubator-nuttx/pull/508/files
...
All new source code need use Apache license, but most people forget this and copy BSD license from other files. It's the right time to enforce the license check like coding style.

The documentation of the script tools is poor. There are no examples of what the arguments take as values.

Do you think the following help isn't enough?

./tools/checkpatch.sh -h
USAGE: ./tools/checkpatch.sh [options] [list|-]

Options:
-h
-c spell check with codespell(install with: pip install codespell
-r range check only (used with -p and -g)
-p <patch list> (default)
-g <commit list>
-f <file list>
-  read standard input mainly used by git pre-commit hook as below:
   git diff --cached | ./tools/checkpatch.sh -

If not, please enhance the description so everyone can get the benefit.

Forcing a user to decodes the script when it is not necessary is not inviting. Why is it it tools/configure.sh imx-1060evk/nsh not `make imx-1060evk/nsh'?

It's hard to teach people invoke make with argument every time. It's also hard to discover how many targets supported by a specific Makefile. BTW, how can you show something like this from Makefile naturally?

./tools/configure.sh -h
USAGE: ./tools/configure.sh [-d] [-s] [-l|m|c|u|g|n] [-a <app-dir>] <board-name>:<config-name>

Where:
  -d enables script debug output
  -s skip the .config/Make.defs existence check
  -l selects the Linux (l) host environment.
  -m selects the macOS (m) host environment.
  -c selects the Windows host and Cygwin (c) environment.
  -u selects the Windows host and Ubuntu under Windows 10 (u) environment.
  -g selects the Windows host and MinGW/MSYS environment.
  -n selects the Windows host and Windows native (n) environment.
  Default: Use host setup in the defconfig file
  Default Windows: Cygwin
  -a <app-dir> is the path to the apps/ directory, relative to the nuttx
     directory
  <board-name> is the name of the board in the boards directory
  configs/<config-name> is the name of the board configuration sub-directory

A lot of pieces all over the place is fine jut keep them out the the users face and have reasonable granularity.

The correct version is always the latest one on the master, we need rebase/cherry-pick our patch with the master anyway, why don't we run checkpatch.sh before sending PR?

No we do not. I explained this already (in email and comments) that work flow is a waste of time and effort with back and forth chery-picking. You do not have a good solution.

[davids5]

@xiaoxiang781216 Clearly we are talking past each other here. I am positive we both have the same goal: to make NuttX better. But the POV are myopic. Possibly the language barrier is in the way. I say this because have been trying to express to you to add the "make helpers" that invokes the script with the arguments and uses make commands to get the params.

You can see this thing like this in action here:

https://github.com/PX4/Firmware/blob/master/Makefile#L321-L329

I already suggest you provide a patch

I want to make sure this subtle point is not getting lost" a patch and a PR are 2 different work flows.

I will not be providing any "patches". Now that the project has matured to using github. I will be providing PRs.

I already suggest you provide a patch to add check_format(actually check_patch may a better name) in Makefile, what I oppose is to call nxstyle directly in Makefile. It's better to call checkpatch.sh

We have been agreeing on this all along: Yes run the script for all of the functions, but do it from make with simple commands that do pieces of it AND the whole thing. make check_contrib is a better name for doing all of them it is not a patch or a PR.

The problem I see with me doing the changes is that I accept that git is part of the work flow. I would not ignore it nor would I ignore linux tools. (this is my myopic POV)

The whole non git and patch work flow is of zero value to me. So therefore I have not made changes to the makefile. I will be happy to do it, but we need buy-in from the PPMC that we are moving to a modern workflow and deprecating the less productive workflows and tools. With containers and VM dragging along support old tool, OS, and build environments puts and undo burden on the project and holds it back from using best practices.

so devloeper can get the same result in his machine as the github CI

But they will not unless they use ONLY the workflow you use, in the order you do you work. This is because they will not have the correct version of nxstyle. This needs to be fixed.

I would not choose your work flow, as it is a waste of time cherry-pick back and forth. I also would never use patches because we now have a much better tools than that. Did you see where Greg asked for the SPI patch to be a submitted as a PR?

If not, please enhance the description so everyone can get the benefit.

I wish I could, but it is not my tool and I am not going to reverse engineer it. I would ask you to have the person that wrote it add examples in this issue of all the commands with real arguments. I will be happy to do a PR form that if will help word it well.

BTW, how can you show something like this from Makefile naturally?

here is a start for you.
https://github.com/PX4/Firmware/blob/master/Makefile#L471-L489

I would suggest you install PX4 on ubuntu using the scriopt "ubuntu_sim_nuttx.sh: ubuntu_sim.sh + NuttX tools" found here https://dev.px4.io/v1.9.0/en/setup/dev_env_linux_ubuntu.html

and see how easy it is as a N00B to build, check the format of code, and even format code. Then you
may have a better understanding of what I have been saying about the user experience and removing barriers.

Mess up some c and h files formatting, then run
git diff
make check_format
make format
git diff

For an example of building targets try this:

make px4_<hit the tab key>

then try
`make px4_fmu-v5 menuconfig"
make some changes
then do a git diff.

All the stuff a new user has to do with nuttx can be simpler and less pron to to errors or loss of work.

Have you ever lost all you work in NuttX because you edited the copied or linked file form arch/chip?

Try an edit on a file in the bulid dir after making, In PX4 it will not get lost.

Build 3 targets. You can also see the advantages of the out of tree build.

[xiaoxiang781216]

@xiaoxiang781216 Clearly we are talking past each other here. I am positive we both have the same goal: to make NuttX better. But the POV are myopic. Possibly the language barrier is in the way. I say this because have been trying to express to you to add the "make helpers" that invokes the script with the arguments and uses make commands to get the params.

You can see this thing like this in action here:

https://github.com/PX4/Firmware/blob/master/Makefile#L321-L329

Please tell me how can I check a specific commit id or all source files in a directory? make check_format? but it check all source files:

check_format:
	$(call colorecho,'Checking formatting with astyle')
	@"$(SRC_DIR)"/Tools/astyle/check_code_style_all.sh
	@cd "$(SRC_DIR)" && git diff --check

I have more experience with Makefile than you imagination: Makefile is a good tool but the shell script is better in some case. There is method to pass the runtime argument(e..g. commit id or directory) to Makefile, but the syntax is very urgly.

I already suggest you provide a patch

I want to make sure this subtle point is not getting lost" a patch and a PR are 2 different work flows.

I will not be providing any "patches". Now that the project has matured to using github. I will be providing PRs.

My patch mean PR here, do you saw I send any patch in email list after NuttX enter incubation? Actually I suggest that people is better to create PR many time in email list:
https://lists.apache.org/thread.html/r7583a3287ee74017e1d8d79be0306382d0e96764bea1721e1ec60f10%40<dev.nuttx.apache.org>

I already suggest you provide a patch to add check_format(actually check_patch may a better name) in Makefile, what I oppose is to call nxstyle directly in Makefile. It's better to call checkpatch.sh

We have been agreeing on this all along: Yes run the script for all of the functions, but do it from make with simple commands that do pieces of it AND the whole thing. make check_contrib is a better name for doing all of them it is not a patch or a PR.

That's fine if you want check_contrib, I just require to call checkpatch.sh.

The problem I see with me doing the changes is that I accept that git is part of the work flow. I would not ignore it nor would I ignore linux tools. (this is my myopic POV)

The whole non git and patch work flow is of zero value to me. So therefore I have not made changes to the makefile. I will be happy to do it, but we need buy-in from the PPMC that we are moving to a modern workflow and deprecating the less productive workflows and tools. With containers and VM dragging along support old tool, OS, and build environments puts and undo burden on the project and holds it back from using best practices.

You can send a VOTE to not support other version control tool.

so devloeper can get the same result in his machine as the github CI

But they will not unless they use ONLY the workflow you use, in the order you do you work. This is because they will not have the correct version of nxstyle. This needs to be fixed.

No, the patch must pass the precheck(checkpatch.sh) no matter what's workflow they use if they want to upstream their work. And checkpatch.sh must be the last version in master branch.

I would not choose your work flow, as it is a waste of time cherry-pick back and forth. I also would never use patches because we now have a much better tools than that.

How do you know my internal workflow? Xiaomi never use github internally at all, could you tell me how can I send PR inside the company if we have internal tooling? So please don't make any assumption that my workflow is bad than yours.

Did you see where Greg asked for the SPI patch to be a submitted as a PR?

Why do you ignore my same question? I ask more than Greg. I have to list here to remember you again:
https://lists.apache.org/thread.html/r7583a3287ee74017e1d8d79be0306382d0e96764bea1721e1ec60f10%40<dev.nuttx.apache.org>

If not, please enhance the description so everyone can get the benefit.

I wish I could, but it is not my tool and I am not going to reverse engineer it.

The code is there, why you need reverse engineer?

I would ask you to have the person that wrote it add examples in this issue of all the commands with real arguments. I will be happy to do a PR form that if will help word it well.

The help usage is very clear. If you think it isn't enough plesae send a PR, thanks.

BTW, how can you show something like this from Makefile naturally?

here is a start for you.
https://github.com/PX4/Firmware/blob/master/Makefile#L471-L489

Here is the output from PX4 make:

make help
Usage: make <target>
Where <target> is one of:

aerotenna_ocpoc
airframe_metadata
airmind_mindpx-v2
all
all_config_targets
all_default_targets
alt_firmware
atlflight_eagle
atlflight_excelsior
auav_x21
av_x-v1
bitcraze_crazyflie
check
check_format
check_rtps
clang-tidy
clang-tidy-fix
clang-tidy-quiet
clean
coverity_scan
cppcheck
distclean
doxygen
excelsior_rtps
format
gazeboclean
help
holybro_durandal-v1
holybro_kakutef7
intel_aerofc-v1
list_config_targets
misc_qgc_extra_firmware
modalai_fc-v1
module_documentation
mro_ctrl-zero-f7
nxp_fmuk66-v3
omnibus_f4sd
parameters_metadata
parrot_bebop
px4_cannode-v1
px4_fmu-v2
px4_fmu-v3
px4_fmu-v4
px4_fmu-v4pro
px4_fmu-v5
px4_fmu-v5x
px4_io-v2
px4_metadata
px4_sitl
px4_sitl_default-clang
px4fmu_firmware
python_coverage
qgc_firmware
quick_check
rostest
rostest_run
scan-build
shellcheck_all
sizes
submodulesclean
submodulesupdate
tests
tests_avoidance
tests_coverage
tests_mission
tests_mission_coverage
tests_offboard
uorb_graphs
uvify_core
validate_module_configs

Or, make <config_target> [<make_target(s)>]
Use 'make list_config_targets' for a list of configuration targets.

Do you think it's clear than this:

./tools/checkpatch.sh -h
USAGE: ./tools/checkpatch.sh [options] [list|-]

Options:
-h
-c spell check with codespell(install with: pip install codespell
-r range check only (used with -p and -g)
-p <patch list> (default)
-g <commit list>
-f <file list>
-  read standard input mainly used by git pre-commit hook as below:
   git diff --cached | ./tools/checkpatch.sh -

If you have time please help PX4 community improve the above help usage, thanks.

I would suggest you install PX4 on ubuntu using the scriopt "ubuntu_sim_nuttx.sh: ubuntu_sim.sh + NuttX tools" found here https://dev.px4.io/v1.9.0/en/setup/dev_env_linux_ubuntu.html

and see how easy it is as a N00B to build, check the format of code, and even format code. Then you
may have a better understanding of what I have been saying about the user experience and removing barriers.

Mess up some c and h files formatting, then run
git diff
make check_format
make format
git diff

For an example of building targets try this:

make px4_<hit the tab key>

then try
`make px4_fmu-v5 menuconfig"
make some changes
then do a git diff.

All the stuff a new user has to do with nuttx can be simpler and less pron to to errors or loss of work.

Let me reemphasize again:
I never refuse check_format/check_contrib in Makefile!
If I said anything about this, please give me a link. What I refuse is to call nxsytle directly in Makefile.

Have you ever lost all you work in NuttX because you edited the copied or linked file form arch/chip?

No, I never lose my work. NuttX just use link.

Try an edit on a file in the bulid dir after making, In PX4 it will not get lost.

Why you complain NuttX make you lose the work?

Build 3 targets. You can also see the advantages of the out of tree build.

Please contribute your out of tree solution.
We use out of tree build, we have more complicated case than you: we have one chipset run three NuttX image on different MCU/DSP with different configuration.

[davids5]

I have more experience with Makefile than you imagination: Makefile is a good tool but the shell script is better in some case. There is method to pass the runtime argument(e..g. commit id or directory) to Makefile, but the syntax is very urgly.

I am sure you do and are very skilled at it. I think this is why it is so hard for you to gasp that a NOOB needs helper. This I all I am asking you to consider. Solve the simple cases for them, and they learn and grow.

Please tell me how can I check a specific commit id or all source files in a directory? make check_format? but it check all source files:

You can not - that is not what it is for. This is not a replacement for the script it is a tool for the first time contributor (We want to to grow the project) The script does the git commands on the branch
to get the names of the change files and checks those files.

A precommit hook would be different.

That's fine if you want check_contrib, I just require to call checkpatch.sh.
Agreed!

My patch mean PR here,

I am just asking you to think about the newcomers to the project. You and I understand your usage in context. But calling a PR a patch will get you patches emailed to you and confuse people.

Do you think it's clear than this:

Yes it tells you what to type to make the command work and has tab-completion.

How do you know my internal workflow? Xiaomi never use github internally at all, could you tell me how can I send PR inside the company if we have internal tooling? So please don't make any assumption that my workflow is bad than yours.

I do not: All I know is you told me I have to do: cherry-pick (or rebase) to master run the tools, fix the problems and submit. But If I cherry-pick to master, those commits came from somewhere and that STABLE branch is now out of syncing with ONLY the changes I am upstreaming. This mean you have to bring those commits back to STABLE.

Compare that to:

run the tools from master on your branch fix the CS issue. Cherry-pick and submit.

It is better because there is no back and forth cherry-picking.

Why do you ignore my same question? I ask more than Greg. I have to list here to remember you again:

Sorry the link is bad. If you use markup it will not get mangled.

The code is there, why you need reverse engineer?

Sorry that was bad wording on my part. The elegant bash script that was submitted was to difficult for me to quickly decipher to find what the arguments should have been. I did not know we could pass HEAD to it.

Aging this fall into the category of the too skilled assuming their knowledge is common knowledge and , not understanding the needs of the newcomer.

Please contribute your out of tree solution.

It comes with cmake and nija - So this is not going to happen any time soon.

@xiaoxiang781216 What I am telling you is you are a very brilliant and skilled engineer, and need to consider making the project more inviting to those less skilled than you.

[xiaoxiang781216]

Let's close this until nxstyle is fully ready.