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/06 02:51:29 UTC

[GitHub] [incubator-nuttx] yamt opened a new pull request #448: Add a sample of git pre-commit hook

yamt opened a new pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448
 
 
   

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r388691596
 
 

 ##########
 File path: tools/git-hooks/pre-commit
 ##########
 @@ -0,0 +1,30 @@
+#!/bin/sh
+
+############################################################################
+# tools/git-hooks/pre-commit
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# This file is intended to be used as .git/hooks/pre-commit
+
+if ! type nxstyle > /dev/null 2>&1; then
 
 Review comment:
   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)

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389473361
 
 

 ##########
 File path: tools/git-hooks/pre-commit
 ##########
 @@ -0,0 +1,30 @@
+#!/bin/sh
+
+############################################################################
+# tools/git-hooks/pre-commit
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# This file is intended to be used as .git/hooks/pre-commit
+
+if ! type nxstyle > /dev/null 2>&1; then
 
 Review comment:
   @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>

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] yamt commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

yamt commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r388695824
 
 

 ##########
 File path: tools/git-hooks/pre-commit
 ##########
 @@ -0,0 +1,30 @@
+#!/bin/sh
+
+############################################################################
+# tools/git-hooks/pre-commit
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# This file is intended to be used as .git/hooks/pre-commit
+
+if ! type nxstyle > /dev/null 2>&1; then
 
 Review comment:
   i was not aware of checkpatch.sh.
   personally i'm not comfortable to run make in git hooks.

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389686312
 
 

 ##########
 File path: tools/git-hooks/pre-commit
 ##########
 @@ -0,0 +1,30 @@
+#!/bin/sh
+
+############################################################################
+# tools/git-hooks/pre-commit
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# This file is intended to be used as .git/hooks/pre-commit
+
+if ! type nxstyle > /dev/null 2>&1; then
 
 Review comment:
   > 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?
   
   > 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.
   
   > 
   > 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? 

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] xiaoxiang781216 closed pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 closed pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448
 
 
   

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r388699113
 
 

 ##########
 File path: tools/git-hooks/pre-commit
 ##########
 @@ -0,0 +1,30 @@
+#!/bin/sh
+
+############################################################################
+# tools/git-hooks/pre-commit
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# This file is intended to be used as .git/hooks/pre-commit
+
+if ! type nxstyle > /dev/null 2>&1; then
 
 Review comment:
   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?

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389369776

##########
File path: tools/git-hooks/pre-commit
##########
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+############################################################################
+# tools/git-hooks/pre-commit
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership. The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# This file is intended to be used as .git/hooks/pre-commit
+
+if ! type nxstyle > /dev/null 2>&1; then

Review comment:
> > 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?
>
> +1 - have a separate script for assing the hook

This patch looks good, except it need to invoke checkpatch.sh to do the real work, actually we has the similar pre-commit in our local git:
```
#!/usr/bin/env bash
# tools/pre-commit
# git hook to run check-patch on the output and stop any commits
# that do not pass. Note, only for git-commit, and not for any of the
# other scenarios
#
# Copyright 2010 Ben Dooks, <be...@fluff.org>

if git rev-parse --verify HEAD 2>/dev/null >/dev/null
then
against=HEAD
else
# Initial commit: diff against an empty tree object
against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
fi

git diff --cached $against -- | ./tools/checkpatch.sh -
```
Why we don't upstream our pre-commit and suggest to generate it dynamiclly instead? Because pre-commit is a very special script, it must put into .git/hooks/ to work as expect, the copy in nuttx/tools can't work standalone but give user the bad expression. If you look at Linux kernel codebase, you can find checkpatch.pl but no pre-commit, it may one of reason.
But anyway, as I said before ,it isn't a big issue.

> I would add the formatting to make "make check_format " - Hide the tooling from the user.

@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.

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] liuguo09 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

liuguo09 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389367902

Review comment:
checkpatch.sh is originally designed in mind to be easily integrated into pre-commit hook. As the script usage shows: git diff --cached | ./tools/checkpatch.sh -
IMO, I prefer reuse checkpatch.sh.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r388709203
 
 

 ##########
 File path: tools/git-hooks/pre-commit
 ##########
 @@ -0,0 +1,30 @@
+#!/bin/sh
+
+############################################################################
+# tools/git-hooks/pre-commit
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# This file is intended to be used as .git/hooks/pre-commit
+
+if ! type nxstyle > /dev/null 2>&1; then
 
 Review comment:
   It's fine to add pre-commit in tools folder or generate it from checkpatch.sh, the key point is the pre-commit need to forward the real action to checkpatch.sh because:
   1.We will continue to enhance the workflow, especially in the current(initial) phase, it is very important that we can modify the workflow in one place.
   2.pre-commit normally need copy to nuttx/.git/hooks and apps/.git/hooks, so it's hard to update it automatically through git pull.
   And If something in checkpatch.sh isn't good, we can send PR to enhance it, then all caller get the update autoamtically.

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] davids5 commented on issue #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

davids5 commented on issue #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#issuecomment-597674843
 
 
   >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.
    

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r390074355
 
 

 ##########
 File path: tools/git-hooks/pre-commit
 ##########
 @@ -0,0 +1,30 @@
+#!/bin/sh
+
+############################################################################
+# tools/git-hooks/pre-commit
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# This file is intended to be used as .git/hooks/pre-commit
+
+if ! type nxstyle > /dev/null 2>&1; then
 
 Review comment:
   > @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?
   https://github.com/apache/incubator-nuttx/pull/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.
   
   

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389473361
 
 

 ##########
 File path: tools/git-hooks/pre-commit
 ##########
 @@ -0,0 +1,30 @@
+#!/bin/sh
+
+############################################################################
+# tools/git-hooks/pre-commit
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# This file is intended to be used as .git/hooks/pre-commit
+
+if ! type nxstyle > /dev/null 2>&1; then
 
 Review comment:
   @yamt you can run checkpatch.sh with -r option to ensure your modification don't introduce the new coding style problem.

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

davids5 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389883624

Review comment:
@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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on issue #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#issuecomment-599450523
 
 
   Let's close this until nxstyle is fully ready.

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] yamt commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

yamt commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389467682

Review comment:
after having this in my pre-commit hook for a few days, i'm seeing it more harmful than useful. at least we should not recommend it for "newbies" until the whole tree passes nxstyle.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

davids5 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389883624

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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389686312
 
 

 ##########
 File path: tools/git-hooks/pre-commit
 ##########
 @@ -0,0 +1,30 @@
+#!/bin/sh
+
+############################################################################
+# tools/git-hooks/pre-commit
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# This file is intended to be used as .git/hooks/pre-commit
+
+if ! type nxstyle > /dev/null 2>&1; then
 
 Review comment:
   > 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? 

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389369776
 
 

 ##########
 File path: tools/git-hooks/pre-commit
 ##########
 @@ -0,0 +1,30 @@
+#!/bin/sh
+
+############################################################################
+# tools/git-hooks/pre-commit
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# This file is intended to be used as .git/hooks/pre-commit
+
+if ! type nxstyle > /dev/null 2>&1; then
 
 Review comment:
   > > 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?
   > 
   > +1 - have a separate script for assing the hook
   
   This patch looks good, except it need to invoke checkpatch.sh to do the real work, actually we has the similar pre-commit in our local git:
   ```
   #!/usr/bin/env bash
   # tools/pre-commit
   # git hook to run check-patch on the output and stop any commits
   # that do not pass. Note, only for git-commit, and not for any of the
   # other scenarios
   #
   # Copyright 2010 Ben Dooks, <be...@fluff.org>
   
   if git rev-parse --verify HEAD 2>/dev/null >/dev/null
   then
           against=HEAD
   else
           # Initial commit: diff against an empty tree object
           against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
   fi
   
   git diff --cached $against -- | ./tools/checkpatch.sh -
   ```
   Why we don't upstream our pre-commit and suggest to generate it dynamiclly instead? Because pre-commit is a very special script, it must put into .git/hooks/ to work as expect, the copy in nuttx/tools can't work standalone but give user the bad expression. If you look at Linux kernel codebase, you can find checkpatch.pl but no pre-commit, it may one of reason.
   But anyway, as I said before ,it isn't a big issue.
   
   > I would add the formatting to make "make check_format " - Hide the tooling from the user.
   
   @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:
      1.spell check
      2.copyright check
      3.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?
   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.
   

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r388698278
 
 

 ##########
 File path: tools/git-hooks/pre-commit
 ##########
 @@ -0,0 +1,30 @@
+#!/bin/sh
+
+############################################################################
+# tools/git-hooks/pre-commit
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# This file is intended to be used as .git/hooks/pre-commit
+
+if ! type nxstyle > /dev/null 2>&1; then
 
 Review comment:
   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.

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

davids5 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389099219

Review comment:
>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?

+1 - have a separate script for assing the hook
I would add the formatting to make "make check_format " - Hide the tooling from the user.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [incubator-nuttx] yamt commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

yamt commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r388701099
 
 

 ##########
 File path: tools/git-hooks/pre-commit
 ##########
 @@ -0,0 +1,30 @@
+#!/bin/sh
+
+############################################################################
+# tools/git-hooks/pre-commit
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+# This file is intended to be used as .git/hooks/pre-commit
+
+if ! type nxstyle > /dev/null 2>&1; then
 
 Review comment:
   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?
   

----------------------------------------------------------------
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

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

xiaoxiang781216 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389369776

if git rev-parse --verify HEAD 2>/dev/null >/dev/null
then
against=HEAD
else
# Initial commit: diff against an empty tree object
against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
fi

> I would add the formatting to make "make check_format " - Hide the tooling from the user.

@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:
1.spell check
2.copyright check
3.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 you can 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

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

[GitHub] [incubator-nuttx] davids5 commented on issue #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

davids5 commented on issue #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#issuecomment-597048769

@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](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.

> 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #448: Add a sample of git pre-commit hook

Posted by GitBox <gi...@apache.org>.

davids5 commented on a change in pull request #448: Add a sample of git pre-commit hook
URL: https://github.com/apache/incubator-nuttx/pull/448#discussion_r389652821

Review comment:
>@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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services