You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/03/10 13:51:28 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #448: Add a
sample of git pre-commit hook
xiaoxiang781216 commented on issue #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#issuecomment-597096735
> @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](https://github.com/apache/incubator-nuttx/issues/521) 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services