You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by GitBox <gi...@apache.org> on 2019/12/22 19:47:44 UTC
[GitHub] [incubator-nuttx] davids5 opened a new pull request #2: Master pr
nxstyle improvments
davids5 opened a new pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2
@patacongo -
This will get us off the ground on the formatting tool we can get in to the workfow.
We are going to need your expertise on the the makefile side. (be nice i got it to work ;)
There should be no formatting check changes - it just get us a tool that will work with compiler aware ide/editors.
`make check_format` - will check all the files that differ from `master`
`tools/check_code_style.sh <file name> will check a file
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360728147
##########
File path: tools/Makefile.unix
##########
@@ -192,11 +192,18 @@ NUTTXNAME = nuttx
BIN = $(NUTTXNAME)$(EXEEXT)
all: $(BIN)
-.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean
+.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean check_format
ifeq ($(GIT_DIR),y)
Review comment:
I don't know what QPQ is Quid Pro Quo? I thought that is what Trump does.
We are big on serving the needs of all end-users above anything else and we are big on code quality.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360726228
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
+ INFO,
+ WARN,
+ ERROR,
+ FATAL
+};
+
+const char *class_text[] =
+{
+ "info",
+ "warning",
+ "error",
+ "fatal"
+};
+
+enum file_e {
+ unknown,
+ c_header,
+ c_source,
Review comment:
Good catch. GCC is tolerant of dangling , at the end of enumerations, but other compilers are not. Still not proper programming style in either case.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725573
##########
File path: tools/Makefile.unix
##########
@@ -192,11 +192,18 @@ NUTTXNAME = nuttx
BIN = $(NUTTXNAME)$(EXEEXT)
all: $(BIN)
-.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean
+.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean check_format
ifeq ($(GIT_DIR),y)
Review comment:
For all of these.. same changes should be applied to Makefile.win. No is using Makefile.win now, but we don't want it to get too far out of sync.
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568317600
What is this? How did I do this? What does closed mean? Is that like declined? What was closed?
Do I need to unclose something?
...
Okay I found the button that I must have pushed. Reopened.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360728789
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
Review comment:
Unresolved... I don't see the change.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360726498
##########
File path: tools/Makefile.unix
##########
@@ -192,11 +192,18 @@ NUTTXNAME = nuttx
BIN = $(NUTTXNAME)$(EXEEXT)
all: $(BIN)
-.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean
+.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean check_format
ifeq ($(GIT_DIR),y)
Review comment:
Since it depeands on the existence of a bash script, there is not simple way to implement this in Makefile.win That would involve writing a check_code_style.bat which no one is going to do for a long time. So we can forget that for now.
----------------------------------------------------------------
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 #2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360783003
##########
File path: tools/check_code_style.sh
##########
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
+if [ -z "$1" ]; then
+ FILES=$(git diff master --name-only);
Review comment:
@davids5 we should only check the modified portion, we can't ask contributor to fix the problem not introuced by them.
----------------------------------------------------------------
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] btashton commented on issue #2: Master pr nxstyle
improvments
Posted by GitBox <gi...@apache.org>.
btashton commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568350703
@xiaoxiang781216 You can create a script that you recommend is run as pre-commit, but the actual hooks are outside of the git working directory, so it will never be configured on a fresh clone.
Usually if I want to support this I provide a little script or instructions for people to symlink to the script in the git working directory.
----------------------------------------------------------------
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] patacongo commented on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568540319
I don't think Xaio Xiang's proposal and your implementation are mutually exclusive.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360728732
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
+ @$(TOPDIR)/tools/check_code_style.sh
+ @cd "$(SRC_DIR)" && git diff --check
Review comment:
The simplest solution would be to make the whole check_format target available only ifeq ($(GIT_DIR),y). that would only be conditional logic at 708 and 195. Pretty easy.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360898824
##########
File path: README.txt
##########
@@ -1368,6 +1368,15 @@ Build Targets and Options
Removes derived object files, archives, executables, and temporary
files, but retains the configuration and context files and directories.
+ check_format
+
+ Will run nxstyle to check compliance with the NuttX coding standard on
+ the files the have been changed relative to master. This feature is only
+ available if 1) git is installed and 2) the changed files are located in
+ the local git repository and have been committed. The benefit of this
+ flow is that commits that only effect formatting changes do not over
+ shadow code changes. As of this writting this is a Uinx only feature.
+
Review comment:
Uinx->Unix
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568537008
> But it's very helpful to issue the above command in the script, because many user even expert don't know this trick.
There are also cases where scripts in tools/ build the binaries that the scipt needs. This makes the scripts nicely self-contained.
Examples... Logic assocated with:
tools/mkconfigvars.sh:KCONFIG2MAKEFILE=Makefile.host
tools/refresh.sh:CMPCONFIGMAKEFILE=Makefile.host
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360886535
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
Review comment:
Ah! Ok Wild do
----------------------------------------------------------------
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] patacongo commented on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568319546
@Ouss4 Thanks! It said it was open in my page. I must have hit close again? It is kind of goosey but I attribute that to the fact the I am a github noob.
----------------------------------------------------------------
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] patacongo closed pull request #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo closed pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568537855
Sorry about the Open/Close/Open/Close. Not exactly sure what I am doing. I suspect that it is the "Close and Comment" button next to the "Comment" button. I am used to hitting a "Cancel" button at that position.
Grrr.. still says closed. Clicking open the pull requests menu page does seem to do it.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725490
##########
File path: tools/nxstyle.c
##########
@@ -477,7 +543,7 @@ int main(int argc, char **argv, char **envp)
(strcmp(line, " * Private Functions\n") == 0 ||
strcmp(line, " * Public Functions\n") == 0))
{
- //fprintf(stderr, "Functions begin at line %d:%d\n", lineno, n);
+ //ERROR("Functions begin", lineno, n);
bfunctions = true;
Review comment:
Same as above: Remove or use #if 0 with comment. No C++ // comments.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360730399
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
Review comment:
Description of all new make targets should be included in the top-level Makefile, including a description of what the make target does.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725723
##########
File path: tools/nxstyle.c
##########
@@ -171,20 +238,21 @@ int main(int argc, char **argv, char **envp)
}
else
{
- fprintf(stderr, "Invalid number of arguments\n");
+ FATAL("Invalid number of arguments\n");
show_usage(argv[0], 1);
}
instream = fopen(filename, "r");
if (!instream)
{
- fprintf(stderr, "Failed to open %s\n", argv[1]);
+ FATALFL("Failed to open", argv[1]);
return 1;
}
/* Are we parsing a header file? */
hdrfile = false;
+ g_file_type = unknown;
ext = strrchr(filename, '.');
Review comment:
Something wierd with the vertical alignment. Either go with vertically aligned = signs, or remove spaces and go with the ragged look. Either is fine, but not both.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360730444
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
Review comment:
Unresolaved again: I still don't see the change
----------------------------------------------------------------
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] patacongo commented on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568528569
This will build all of the tools including a dozen or so that you don't need and probably will never need:
$(MAKE) -C tools -f Makefile.host
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725892
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
+ INFO,
+ WARN,
+ ERROR,
+ FATAL
+};
+
+const char *class_text[] =
+{
+ "info",
+ "warning",
+ "error",
+ "fatal"
+};
+
+enum file_e {
+ unknown,
+ c_header,
+ c_source,
+};
+
+/****************************************************************************
+ * Private data
+ ****************************************************************************/
+
+int g_status = 0;
+char *g_file_name = "";
+enum file_e g_file_type = unknown;
Review comment:
```suggestion
static enum file_e g_file_type = unknown;
```
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568317600
What is this? How did I do this? What does closed mean? Is that like declined? What was closed?
Do I need to unclose something?
----------------------------------------------------------------
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] patacongo commented on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568309238
I added Xiao Xiang to the reviewers. We need to have a lot of concurrence here since this PR implements a portion of the workflow while there no workflow requirements in place. I think this piece of the workflow is not so controversial, I would like to have Xiang provide his input as well. Xiang is the most authorative person on the build system.
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725585
##########
File path: tools/nxstyle.c
##########
@@ -477,7 +543,7 @@ int main(int argc, char **argv, char **envp)
(strcmp(line, " * Private Functions\n") == 0 ||
strcmp(line, " * Public Functions\n") == 0))
{
- //fprintf(stderr, "Functions begin at line %d:%d\n", lineno, n);
+ //ERROR("Functions begin", lineno, n);
bfunctions = true;
Review comment:
ehmm @patacongo that came from master
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360728789
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
Review comment:
Unresolved... I don't see the change.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360727266
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
+ @$(TOPDIR)/tools/check_code_style.sh
+ @cd "$(SRC_DIR)" && git diff --check
Review comment:
There is another problem here: You must account for that fact that .git/ is NOT present in most peoples environment. Please make the last line conditioned on #ifeq ($(GIT_DIR),y) or you will break 80% of the nuttx user's builds!
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360803384
##########
File path: tools/check_code_style.sh
##########
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
Review comment:
I was looking at this as in incremental steps with reusable pieces that we are free to evolve.
The real value add in this PR is the ability to fix in ones editor the CS violations.
NuttX has no complete CS solution. It is impossible to enforce a CS without tools that work 100% and is not go-no-go. for CI and pr-commit hooks.
We should not expect professional developers and new members of the community to have to read 10 emails on how each one of the current tools do not work and have them understand that the indent.sh will destroy a formatted file. What will help is a proper tool like Astyle, Uncurstify, clang-format that can detect and FIX the code allong with a few gold standard files (h,c,Kconfig) to validate it with.
At this point. I think there are there options.
1. We fix the tooling with all your suggestions.
2. We remove the tooling commit (I keep them separate on purpose) until we have the work flow nailed.
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568604222
I would like to close this now. I do not think it should be incorporated into the NuttX repositories now. It deals with specific logic for the NuttX workflow. We need to have a defintiion for the workflow before we can consider including it.
Then we need to bring all of the other people on board who are tasked with working with the NuttX: Haitao Liu and Anthony Merlino. A slam dunk of PX4 workflow is not acceptable.
We should revisit this in the future when there is an agreed upon work flow and when all participants in the development of the NuttX workflow are on board and have the opportunity to voice their support or concerns.
This time, I did not accidentally hit the button. I really did intend to close the PR. But let's not consider it finally closed. It is closed until can define the workflow requirements and get all of the relevant parties involved.
----------------------------------------------------------------
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] Ouss4 commented on issue #2: Master pr nxstyle
improvments
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568318466
Yes close without merging is like decline.
It's still closed btw.
----------------------------------------------------------------
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 issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568512309
> > @btashton @davids5 What I mean here is that we don't need modify Makefile, is engouh to add a script invoke nxstyle(maybe build it too) to check the style for specific file/files or commit/commits.
> > Then we instruct people integrate this script into pre-commit hook to enable the check automatically. Like:
> > https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl
> > Nobody try to integrate checkpatch.pl into Linux build system, since developer can run checkpatch.pl directly to:
> > 1.The bunch of specified files
> > 2.The bunch of specified commits
> > 3.The current living change
> > 4.Integrate to pre-commit hook
>
> > nxstyle(maybe build it too)
>
> That is make's job.
>
Yes, but we already have the rule in tools/Makefile.host for generating nxstyle binary, the question is where should we trigger the target. It's better to let the script trigger the buildotherwise user can't invoke the tool outside the make environment(e.g. from jenkins job, precommit hook or command line).
> > It's much flexible than make check_format since we can pass the different argument to script form command line, but the same thing is hard to do for Makefile.
>
> I agree - but adding it to the make file has some benefits and it does not prevent any entity (user, ci, tools) from running the script directly.
>
> There are a few subtle points here from a non-power users and system POV.
>
> We want to grow the community it is make for easy of use fot N00Bs
>
> 1. Most editors and ide's have make integration.
> 2. First normal form and abstraction.
>
> If we add the formatting targets to the `MakeFile` it is top down and use-able by the users, CI and other tools.
>
> `make check_format`
> `make check _format_all`
> and for the user (When, we have a decent tool)
> `make format` - that fixes the issues
>
Sorry, I make some confusion here. The build target to check/fix the style issuse is good idea, but we need also consider other use case too(e.g. command line, precommit hook and jenkins job). So it's better to develop the script first and keep all invoking environment in mind and equally(don't bias on make too earily). Once the script is ready, the integrattion is a very simple task.
> As of today `nxstyle` lives in `tools` and in tree. It may need to be moved. Think about the best practices with git when the release branch's nxstyle != master's nxstyle and master is correct nxstye (or a conf file for a real tool).
>
> It will level room for a whole class of errors we can avoid with a simple Workflow.
>
> Also consider the change to tools like when nxstyle++ is written or we use clang-format. Do you want to update the location and process everywhere: in the CI repos, containers and ./tools or do you want to update the makefile?
Yes, I agree we need a shell script here, so we can change the implentation without influencing any user. But we shouldn't move the script to any other location: tools/ is a good place.
>
> @btashton , @xiaoxiang781216 Please set me straight if I am off course or too myopic.
> The things we get wrong now will become technical-debt. Granted at this point it in time it is small but when there are 300 committers on nuttx it will be a problem 300 times lager.
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725880
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
+ INFO,
+ WARN,
+ ERROR,
+ FATAL
+};
+
+const char *class_text[] =
+{
+ "info",
+ "warning",
+ "error",
+ "fatal"
+};
+
+enum file_e {
+ unknown,
+ c_header,
+ c_source,
+};
+
+/****************************************************************************
+ * Private data
+ ****************************************************************************/
+
+int g_status = 0;
+char *g_file_name = "";
Review comment:
```suggestion
static char *g_file_name = "";
```
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360727717
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
+ @$(TOPDIR)/tools/check_code_style.sh
+ @cd "$(SRC_DIR)" && git diff --check
Review comment:
@patacongo see [here](https://github.com/apache/incubator-nuttx/pull/2#discussion_r360727667)
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568537855
Sorry about the Open/Close/Open/Close. Not exactly sure what I am doing. I suspect that it is the "Close and Comment" button next to the "Comment" button. I am used to hitting a "Cancel" button at that position.
Grrr.. still says closed. Clicking open the pull requests menu page does NOT seem to do it. Obviously, "a button below the comment box." No not obvious.
----------------------------------------------------------------
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] Ouss4 commented on issue #2: Master pr nxstyle
improvments
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568320150
> I must have hit close again?
I don't know, it never said re-opened, eventhough it should re-open the moment you comment.
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360794087
##########
File path: tools/check_code_style.sh
##########
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
+if [ -z "$1" ]; then
+ FILES=$(git diff master --name-only);
+else
+ FILES=$1
+fi
+
+DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
+for FILE in $FILES; do
+ FILENAME=$(basename $FILE)
+ FILE_EXT="${FILENAME#*.}"
+ if [ -f "$FILE" ]; then
+ if [ "$FILE_EXT" = "c" ] || [ "$FILE_EXT" = "h" ]; then
+ CHECK_FAILED=$(${DIR}/nxstyle $FILE)
+ # if [ -n "$CHECK_FAILED" ]; then
+ # ${DIR}/fix_code_style.sh --quiet < $FILE > $FILE.pretty
+ #
+ # echo
+ # git --no-pager diff --no-index --minimal --histogram --color=always $FILE $FILE.pretty
+ # rm -f $FILE.pretty
Review comment:
@patacongo - we can add a test for git. and address all you other concerns, that is not a problem.
But I think you should think about this again over morning coffee. This does not break anyone's build.* It is a makefile helper and command line option to help them ONLY check the files they changed. They will need git installed to do this.
@patacongo It is True it will not work on window util someone who works on windows make the changes. I do not work on windows so I will not do this. Because I will not be able to test it - and it will be wrong. I do not add Value there. - You could.
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725833
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
+ INFO,
+ WARN,
+ ERROR,
+ FATAL
+};
+
+const char *class_text[] =
+{
+ "info",
+ "warning",
+ "error",
+ "fatal"
+};
+
+enum file_e {
Review comment:
```suggestion
enum file_e
{
```
----------------------------------------------------------------
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] patacongo commented on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568527990
Because most of them are not needed for the build. And it just wastes build cycles building tools that are not used. You ran into this with nxstyle.exe only because it was not on used in the build before your change. The build target in the Makefile is nice because it only builds the executable if it will be needed.
----------------------------------------------------------------
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 #2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360904297
##########
File path: tools/Makefile.unix
##########
@@ -324,6 +324,14 @@ tools/cnvwindeps$(HOSTEXEEXT):
# $(Q) echo "No .config file found, creating one"
# $(Q) tools/initialconfig$(HOSTEXEEXT)
+# make it easy to see what is happening
+COLOR_BLUE = \033[0;94m
Review comment:
@davids5 I would prefer we work on check_code_style.sh first, and forget Makefile.unix at all.
Why? this avoid we couple the script logic with Makefle, for example:
1.The above implementation make the standalone script can't colorize the output.
2.The dependence rule can't work if user invoke the script from shell directly.
The best aproach is that we focus on the script development and make it work from command line as we want, then we can integrate the script either as the make target or precommit hook. Otherwise, the script will do a major rework for the standalone case.
Invoking the script from command line is the key feature for this tool, becasue we need integrate this tool into various workflow(e.g. Makefile, precommit hook, jenkins job).
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360905760
##########
File path: tools/Makefile.unix
##########
@@ -324,6 +324,14 @@ tools/cnvwindeps$(HOSTEXEEXT):
# $(Q) echo "No .config file found, creating one"
# $(Q) tools/initialconfig$(HOSTEXEEXT)
+# make it easy to see what is happening
+COLOR_BLUE = \033[0;94m
Review comment:
That actually makes a lot of sense, is simpler, and sounds more flexible. I would tend to support Xiao Xiang's argument.
Since this has taken a different twist, should we unresolve the conversation?
----------------------------------------------------------------
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] patacongo closed pull request #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo closed pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360886392
##########
File path: tools/check_code_style.sh
##########
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
+if [ -z "$1" ]; then
+ FILES=$(git diff master --name-only);
+else
+ FILES=$1
+fi
+
+DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
+for FILE in $FILES; do
+ FILENAME=$(basename $FILE)
+ FILE_EXT="${FILENAME#*.}"
+ if [ -f "$FILE" ]; then
+ if [ "$FILE_EXT" = "c" ] || [ "$FILE_EXT" = "h" ]; then
+ CHECK_FAILED=$(${DIR}/nxstyle $FILE)
+ # if [ -n "$CHECK_FAILED" ]; then
+ # ${DIR}/fix_code_style.sh --quiet < $FILE > $FILE.pretty
+ #
+ # echo
+ # git --no-pager diff --no-index --minimal --histogram --color=always $FILE $FILE.pretty
+ # rm -f $FILE.pretty
Review comment:
Done
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725475
##########
File path: tools/nxstyle.c
##########
@@ -477,7 +543,7 @@ int main(int argc, char **argv, char **envp)
(strcmp(line, " * Private Functions\n") == 0 ||
strcmp(line, " * Public Functions\n") == 0))
{
- //fprintf(stderr, "Functions begin at line %d:%d\n", lineno, n);
Review comment:
Commented out code not permitted. Use of C++ // commenting not permitted. Coding standard only permit #if 0 for commented out code WITH an short explanation for why is is commented out.
Otherwise, commented out code should be removed.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360728415
##########
File path: tools/Makefile.unix
##########
@@ -192,11 +192,18 @@ NUTTXNAME = nuttx
BIN = $(NUTTXNAME)$(EXEEXT)
all: $(BIN)
-.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean
+.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean check_format
ifeq ($(GIT_DIR),y)
Review comment:
No one is going to be sending any PRs. No GIT-dependent code is allowed in the build system file unless is is properly conditioned on the existence of .git/
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360727718
##########
File path: tools/check_code_style.sh
##########
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
+if [ -z "$1" ]; then
+ FILES=$(git diff master --name-only);
Review comment:
@patacongo see [here](https://github.com/apache/incubator-nuttx/pull/2#discussion_r360727667)
----------------------------------------------------------------
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] patacongo commented on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568317600
What is this? How did I do this? What does closed mean? What was closed?
----------------------------------------------------------------
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 #2: Master pr nxstyle
improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568538442
LOL - RTM, If we had CI you would have swapped it.
*From:* patacongo [mailto:notifications@github.com]
*Sent:* Monday, December 23, 2019 9:46 AM
*To:* apache/incubator-nuttx
*Cc:* David Sidrane; Mention
*Subject:* Re: [apache/incubator-nuttx] Master pr nxstyle improvments (#2)
Sorry about the Open/Close/Open/Close. Not exactly sure what I am doing. I
suspect that it is the "Close and Comment" button next to the "Comment"
button. I am used to hitting a "Cancel" button at that position.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/apache/incubator-nuttx/pull/2?email_source=notifications&email_token=AAO3BXLEKXJFIQS4QWWO6KLQ2D2NDA5CNFSM4J6NHFG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHRTF7Y#issuecomment-568537855>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAO3BXIINFMHSR22EJODGJLQ2D2NDANCNFSM4J6NHFGQ>
.
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360804008
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
Review comment:
One is enough!
----------------------------------------------------------------
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] patacongo commented on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568537855
Sorry about the Open/Close/Open/Close. Not exactly sure what I am doing. I suspect that it is the "Close and Comment" button next to the "Comment" button. I am used to hitting a "Cancel" button at that position.
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360886136
##########
File path: tools/check_code_style.sh
##########
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
+if [ -z "$1" ]; then
+ FILES=$(git diff master --name-only);
Review comment:
> Similar problem here. You must check if .git/ exists. If it does not, you may not use any git commands. Most users do not have .git/ and this will break for the majority of users.
Done.
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568604222
I would like to close this now. I do not think it should be incorporated into the NuttX repositories now. It deals with specific logic for the NuttX workflow. We need to have a defintiion for the workflow before we can consider including it.
Then we need to bring all of the other people on board who are tasked with working with the NuttX: Haitao Liu and Anthony Merlino. A slam dunk of PX4 workflow is not acceptable.
We should revisit this in the future when there is an agreed upon work flow and when all participants in the development of the NuttX workflow are on board and have the opportunity to voice their support or concerns.
This time, I did not accidentally hit the button. I really did intend to close the 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] davids5 opened a new pull request #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 opened a new pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2
@patacongo -
This will get us off the ground on the formatting tool we can get in to the workfow.
We are going to need your expertise on the the makefile side. (be nice i got it to work ;)
There should be no formatting check changes - it just get us a tool that will work with compiler aware ide/editors.
`make check_format` - will check all the files that differ from `master`
`tools/check_code_style.sh file name` will check a file
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725366
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
+ INFO,
+ WARN,
+ ERROR,
+ FATAL
+};
+
+const char *class_text[] =
+{
+ "info",
+ "warning",
+ "error",
+ "fatal"
+};
+
+enum file_e {
+ unknown,
+ c_header,
+ c_source,
+};
+
+/****************************************************************************
+ * Private data
+ ****************************************************************************/
+
+int g_status = 0;
Review comment:
privatat data should be static
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360798664
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
+ @$(TOPDIR)/tools/check_code_style.sh
+ @cd "$(SRC_DIR)" && git diff --check
Review comment:
Yeah I already had thought of that. I had time to think - but not change it. The days have been very, very long here with the added overhead of ASF.
I would like to ask you to contribute. It is far better for building community to add value and help with what you know.
Understanding that using Github is new to you. I woulds suggest you take these steps to help. It will reduce the back and forth. it is like ifdef rash in code - we should avoid it.
- Be patient - we all that other commitments not just NuttX. Theres is now way for a team to move as quickly as you did on you own.
- Use the [Suggesting Changes on GitHub](https://haacked.com/archive/2019/06/03/suggested-changes/) This makes your comments the solution as works well for the [manini](https://en.wiktionary.org/wiki/manini) stuff.
- Lean to use the mark up. You can say [ditto](https://github.com/apache/incubator-nuttx/pull/2/files#r360726623) And avoid a messy review.
You can then link to the NuttX sites coding standard instead of re typing them all the time.
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360791455
##########
File path: tools/check_code_style.sh
##########
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
+if [ -z "$1" ]; then
+ FILES=$(git diff master --name-only);
Review comment:
@xiaoxiang781216 - Right! If you read [here](https://github.com/apache/incubator-nuttx/pull/2#issue-356115637) I asked for help on the tooling.
In the absence of a workflow I was assuming a best practices "upstream submission" workflow
It assumes the dev has to checkout "upstream master". Then branches it to master-pr-xxxxx. Then cherry picks their fork's commits to it.
At that point the diff is the delta of dev/master-pr-xxxxx to upstream/master and this is checking for that.
I think this mimic `git format-patch master` is that not how you submit?
How do you want this to work (what git commands do you use)?
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360727667
##########
File path: tools/Makefile.unix
##########
@@ -192,11 +192,18 @@ NUTTXNAME = nuttx
BIN = $(NUTTXNAME)$(EXEEXT)
all: $(BIN)
-.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean
+.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean check_format
ifeq ($(GIT_DIR),y)
Review comment:
@patacongo -
All the code review feeback is in. I am done for the day. Please send PR to this branch if you can help fix the Makefiles stuff. and script.
After all I did fix all you CS violations that are on master! We are big on QPQ here!!!
----------------------------------------------------------------
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 #2: Master pr nxstyle
improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568456453
> @btashton @davids5 What I mean here is that we don't need modify Makefile, is engouh to add a script invoke nxstyle(maybe build it too) to check the style for specific file/files or commit/commits.
> Then we instruct people integrate this script into pre-commit hook to enable the check automatically. Like:
> https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl
> Nobody try to integrate checkpatch.pl into Linux build system, since developer can run checkpatch.pl directly to:
> 1.The bunch of specified files
> 2.The bunch of specified commits
> 3.The current living change
> 4.Integrate to pre-commit hook
> nxstyle(maybe build it too)
That is make's job.
> It's much flexible than make check_format since we can pass the different argument to script form command line, but the same thing is hard to do for Makefile.
I agree - but adding it to the make file has some benefits and it does not prevent any entity (user, ci, tools) from running the script directly.
There are a few subtle points here from a non-power users and system POV.
We want to grow the community it is make for easy of use fot N00Bs
1. Most editors and ide's have make integration.
2. First normal form and abstraction.
If we add the formatting targets to the `MakeFile` it is top down and use-able by the users, CI and other tools.
`make check_format`
`make check _format_all`
and for the user (When, we have a decent tool)
`make format` - that fixes the issues
As of today `nxstyle` lives in `tools` and in tree. It may need to be moved. Think about the best practices with git when the release branch's nxstyle != master's nxstyle and master is correct nxstye (or a conf file for a real tool).
It will level room for a whole class of errors we can avoid with a simple Workflow.
Also consider the change to tools like when nxstyle++ is written or we use clang-format. Do you want to update the location and process everywhere: in the CI repos, containers and ./tools or do you want to update the makefile?
@btashton , @xiaoxiang781216 Please set me straight if I am off course or too myopic.
The things we get wrong now will become technical-debt. Granted at this point it in time it is small but when there are 300 committers on nuttx it will be a problem 300 times lager.
----------------------------------------------------------------
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 #2: Master pr nxstyle
improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568402453
> I added Xiao Xiang to the reviewers. We need to have a lot of concurrence here since this PR implements a portion of the workflow while there no workflow requirements in place. I think this piece of the workflow is not so controversial, but I would like to have Xiang provide his input as well. Xiang is the most authorative person on the build system and must be in the loop on build system changes.
@patacongo Thank you!
@xiaoxiang781216 I will add you this organization if you like. Then you can free to push commit here. Or pull this it to upsteam as a **branch** and we can all collaborate on it there. What ever get this in.
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360727720
##########
File path: tools/check_code_style.sh
##########
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
+if [ -z "$1" ]; then
+ FILES=$(git diff master --name-only);
+else
+ FILES=$1
+fi
+
+DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
+for FILE in $FILES; do
+ FILENAME=$(basename $FILE)
+ FILE_EXT="${FILENAME#*.}"
+ if [ -f "$FILE" ]; then
+ if [ "$FILE_EXT" = "c" ] || [ "$FILE_EXT" = "h" ]; then
+ CHECK_FAILED=$(${DIR}/nxstyle $FILE)
+ # if [ -n "$CHECK_FAILED" ]; then
+ # ${DIR}/fix_code_style.sh --quiet < $FILE > $FILE.pretty
+ #
+ # echo
+ # git --no-pager diff --no-index --minimal --histogram --color=always $FILE $FILE.pretty
+ # rm -f $FILE.pretty
Review comment:
@patacongo see [here](https://github.com/apache/incubator-nuttx/pull/2#discussion_r360727667)
----------------------------------------------------------------
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] Ouss4 commented on a change in pull request #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360726048
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
+ INFO,
+ WARN,
+ ERROR,
+ FATAL
+};
+
+const char *class_text[] =
+{
+ "info",
+ "warning",
+ "error",
+ "fatal"
+};
+
+enum file_e {
+ unknown,
+ c_header,
+ c_source,
Review comment:
There should be no dangling comma on the final value of the enumeration
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360732967
##########
File path: tools/check_code_style.sh
##########
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
Review comment:
Missing license header. Must be included on all scripts. Should this be apache or BSD. BSD is safe for now because all of the other files have BSD headers and this will be properly updated to the apache license when that effort begins. It cannot come into the repository with no license.
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568604222
I would like to close this now. I do not think it should be incorporated into the NuttX repositories now. It deals with specific logic for the NuttX workflow. We need to have a defintiion for the workflow before we can consider including it.
Then we need to bring all of the other people on board who are tasked with working with the NuttX: Haitao Liu and Anthony Merlino. A slam dunk of PX4 workflow is not acceptable.
This time, I did not accidentally hid the button. I really did intend to close the 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 issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568532088
> > Yes, but we already have the rule in tools/Makefile.host for generating nxstyle binary, the question is where should we trigger the target. It's better to let the script trigger the buildotherwise user can't invoke the tool outside the make environment(e.g. from jenkins job, precommit hook or command line).
>
> Why is the building of the host tools not part of the top level make process? Shouldn't the host tools be built So that there is a sane environment prior to building the config target?
Because user may run the script without issue any make command first, for example:
The jenkins job could run the script check the style before build to save the time.
As Greg indicate, we can build nxstyle with one line command:
make -C tools -f Makefile.host nxstyle
But it's very helpful to issue the above command in the script, because many user even expert don't know this trick.
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360889472
##########
File path: tools/Makefile.unix
##########
@@ -324,6 +324,14 @@ tools/cnvwindeps$(HOSTEXEEXT):
# $(Q) echo "No .config file found, creating one"
# $(Q) tools/initialconfig$(HOSTEXEEXT)
+# make it easy to see what is happening
+COLOR_BLUE = \033[0;94m
Review comment:
Done. Yes there seems to be the "what you do" and what we have documented - disconnect.
# Please use the "Suggested Change Button or ctr-g"
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725416
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
+ INFO,
+ WARN,
+ ERROR,
+ FATAL
+};
+
+const char *class_text[] =
+{
+ "info",
+ "warning",
+ "error",
+ "fatal"
+};
+
+enum file_e {
+ unknown,
Review comment:
Use the button and you can fix
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568540319
I don't think Xaio Xiang's proposal and your implementation are mutually exclusive.
Correct me if I am misreading things, but I believe that the situation here is that David serves two masters, primarily PX4 and secondarily NuttX. So his objective will always be to submt the optimal PX4 solution and then attempt to bulldoze it through, regardless of what we feel about it. I presume that is why he is so opposed to having high level workflow requirements because they may conflict with PX4.
The may be a good thing, but one side effect that that whatever you may think will probably be ignored. The PX4 code is not open to compromise for NuttX. We will just have to see how all of this works out. Be careful but be open minded.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360726623
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
Review comment:
There is a missing dependency here. tools/nxstyle[.exe] is not build by default but must be explictly built. Can you add
`nxstyle$(EXEEXT):
$(MAKE) -C tools -f Makfile.host nxstyle$(EXEEXT)
check_format: nxstyle$(EXEEXT)
...`
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360726015
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
+ INFO,
+ WARN,
+ ERROR,
+ FATAL
+};
+
+const char *class_text[] =
+{
+ "info",
+ "warning",
+ "error",
+ "fatal"
+};
+
+enum file_e {
+ unknown,
Review comment:
Fix it yourself
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360917765
##########
File path: tools/Makefile.unix
##########
@@ -324,6 +324,14 @@ tools/cnvwindeps$(HOSTEXEEXT):
# $(Q) echo "No .config file found, creating one"
# $(Q) tools/initialconfig$(HOSTEXEEXT)
+# make it easy to see what is happening
+COLOR_BLUE = \033[0;94m
Review comment:
Unresolved only because I think we need to address Xiao Xiang's recommendation in some fashion. It would have been better if this were a separate comment.
----------------------------------------------------------------
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 #2: Master pr nxstyle
improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568446656
Oh I should also mention that the this was from before I added the class of error and treated long lines as warnings in H files.
```
exec < /dev/tty
while true; do
read -p "[post-commit hook] Commit? (Y/n) " yn
if [ "$yn" = "" ]; then
yn='Y'
fi
case $yn in
[Yy] ) exit 0; break;;
[Nn] ) exit 1;;
* ) echo "Please answer y or n for yes or no.";;
esac
done
```
Is not the way this should work it is Just that nxstyle is not mature enough to trust.
----------------------------------------------------------------
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 #2: Master pr nxstyle
improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568445894
> Maybe we can use git pre-commit hook to ensure anyone run nxsytle before commit to his/her local working branch:
> https://eing.github.io/technology/2016/01/28/Git-Pre-Commit-Hooks-Part1/
> And we even can run make in pre-commit.sh to generate the nxsytle.
Yes. I had sent it on the list at one point, Not sure what list....
@xiaoxiang781216 - please consider [this issue](https://github.com/PX4/Firmware/issues/7125) and How it will effect us.
.git/hooks/pre-commit
```
#!/bin/sh
#
# Copyright (c) 2012 - 2019, PX4 Development Team
# All rights reserved.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are met:
#
# * Redistributions of source code must retain the above copyright notice, this
# list of conditions and the following disclaimer.
#
# * Redistributions in binary form must reproduce the above copyright notice,
# this list of conditions and the following disclaimer in the documentation
# and/or other materials provided with the distribution.
#
# * Neither the name of the copyright holder nor the names of its
# contributors may be used to endorse or promote products derived from
# this software without specific prior written permission.
#
# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#
#
# An example hook script to verify what is about to be committed.
# Called by "git commit" with no arguments. The hook should
# # exit with non-zero status after issuing an appropriate message if
# it wants to stop the commit.
#
# To enable this hook, rename this file to "pre-commit".
if git rev-parse --verify HEAD >/dev/null 2>&1
then
against=HEAD
else
# Initial commit: diff against an empty tree object
against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
fi
# If you want to allow non-ascii filenames set this variable to true.
allownonascii=$(git config hooks.allownonascii)
# Redirect output to stderr.
exec 1>&2
# Cross platform projects tend to avoid non-ascii filenames; prevent
# them from being added to the repository. We exploit the fact that the
# printable range starts at the space character and ends with tilde.
if [ "$allownonascii" != "true" ] &&
# Note that the use of brackets around a tr range is ok here, (it's
# even required, for portability to Solaris 10's /usr/bin/tr), since
# the square bracket bytes happen to fall in the designated range.
test $(git diff --cached --name-only --diff-filter=A -z $against |
LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0
then
echo "Error: Attempt to add a non-ascii file name."
echo
echo "This can cause problems if you want to work"
echo "with people on other platforms."
echo
echo "To be portable it is advisable to rename the file ..."
echo
echo "If you know what you are doing you can disable this"
echo "check using:"
echo
echo " git config hooks.allownonascii true"
echo
exit 1
fi
# If there are whitespace errors, print the offending file names and fail.
git diff-index --check --cached $against --
if [ $? -ne 0 ]
then
exit 1
fi
# Check for code style, only in changed files
for i in `git diff --cached --name-only --diff-filter=ACM`
do
echo $i:
FILENAME=$(basename $i)
FILE_EXT="${FILENAME#*.}"
echo FILENAME=$FILENAME
echo FILE_EXT=$FILE_EXT
if [ "$FILE_EXT" = "c" ] || [ "$FILE_EXT" = "h" ]; then
./tools/nxstyle -m 80 $i
fi
echo
done
exec < /dev/tty
while true; do
read -p "[post-commit hook] Commit? (Y/n) " yn
if [ "$yn" = "" ]; then
yn='Y'
fi
case $yn in
[Yy] ) exit 0; break;;
[Nn] ) exit 1;;
* ) echo "Please answer y or n for yes or no.";;
esac
done
```
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725335
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
+ INFO,
Review comment:
Does not follow coding standard: { belows on a separate line)
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360729083
##########
File path: tools/nxstyle.c
##########
@@ -140,7 +207,7 @@ int main(int argc, char **argv, char **envp)
maxline = 78;
filename = argv[1];
-
+ g_file_name = filename;
/* Usage: nxstyle [-m <maxline>] <filename>
Review comment:
There needs to be a blank line before the comment.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360905760
##########
File path: tools/Makefile.unix
##########
@@ -324,6 +324,14 @@ tools/cnvwindeps$(HOSTEXEEXT):
# $(Q) echo "No .config file found, creating one"
# $(Q) tools/initialconfig$(HOSTEXEEXT)
+# make it easy to see what is happening
+COLOR_BLUE = \033[0;94m
Review comment:
That actually makes a lot of sense, is simpler, and sounds more flexible. I would tend to support Xiao Xiang's argument.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360917765
##########
File path: tools/Makefile.unix
##########
@@ -324,6 +324,14 @@ tools/cnvwindeps$(HOSTEXEEXT):
# $(Q) echo "No .config file found, creating one"
# $(Q) tools/initialconfig$(HOSTEXEEXT)
+# make it easy to see what is happening
+COLOR_BLUE = \033[0;94m
Review comment:
Unresolved only because I think we need to address Xiao Xiang's recommendation in some fashion.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725385
##########
File path: tools/nxstyle.c
##########
@@ -61,6 +100,34 @@ static void show_usage(char *progname, int exitcode)
exit(exitcode);
}
+static int message(enum class_e class, const char *text, int lineno, int ndx)
+{
+
Review comment:
Remove this spurious blank line
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568537855
Sorry about the Open/Close/Open/Close. Not exactly sure what I am doing. I suspect that it is the "Close and Comment" button next to the "Comment" button. I am used to hitting a "Cancel" button at that position.
Grrr.. still says closed. Clicking open on the pull requests menu page does NOT seem to do it. Obviously, "a button below the comment box." No not so obvious.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360728147
##########
File path: tools/Makefile.unix
##########
@@ -192,11 +192,18 @@ NUTTXNAME = nuttx
BIN = $(NUTTXNAME)$(EXEEXT)
all: $(BIN)
-.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean
+.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean check_format
ifeq ($(GIT_DIR),y)
Review comment:
I don't know what QPQ is Quid Pro Quo? I thought that is what Trump does.
We are bit on serving the needs of all end-users above anything else and we are big on code quality.
----------------------------------------------------------------
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] Ouss4 commented on a change in pull request #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725448
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
+ INFO,
+ WARN,
+ ERROR,
+ FATAL
+};
+
+const char *class_text[] =
+{
+ "info",
+ "warning",
+ "error",
+ "fatal"
+};
+
+enum file_e {
+ unknown,
+ c_header,
+ c_source,
Review comment:
Enumeration values are always in upper case.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360868178
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
Review comment:
No, I meant README not Makefile. Here:
`1354 Build Targets and Options`
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725908
##########
File path: tools/nxstyle.c
##########
@@ -61,6 +100,34 @@ static void show_usage(char *progname, int exitcode)
exit(exitcode);
}
+static int message(enum class_e class, const char *text, int lineno, int ndx)
+{
+
Review comment:
```suggestion
```
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360728886
##########
File path: tools/nxstyle.c
##########
@@ -171,20 +238,21 @@ int main(int argc, char **argv, char **envp)
}
else
{
- fprintf(stderr, "Invalid number of arguments\n");
+ FATAL("Invalid number of arguments\n");
show_usage(argv[0], 1);
}
instream = fopen(filename, "r");
if (!instream)
{
- fprintf(stderr, "Failed to open %s\n", argv[1]);
+ FATALFL("Failed to open", argv[1]);
return 1;
}
/* Are we parsing a header file? */
hdrfile = false;
+ g_file_type = unknown;
ext = strrchr(filename, '.');
Review comment:
Unresolved. Please make alignment style consistent.
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568540319
I don't think Xaio Xiang's proposal and your implementation are mutually exclusive.
Correct me if I am misreading things, but I believe that the situation here is that David services to masters, primarily PX4 and secondarily NuttX. So his objective will always be to submt the optimal PX4 solution and then attempt to bulldoze it through, regardless of what we feel about it. I presume that is why he is so opposed to having high level workflow requirements because they may conflict with PX4.
The may be a good thing, but one side effect that that whatever you may think will probably be ignored. The PX4 code is not open to compromise for NuttX. We will just have to see how all of this works out. Be careful but be open minded.
----------------------------------------------------------------
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 opened a new pull request #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 opened a new pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2
@patacongo -
This will get us off the ground on the formatting tool we can get in to the workfow.
We are going to need your expertise on the the makefile side. (be nice i got it to work ;)
There should be no formatting check changes - it just get us a tool that will work with compiler aware ide/editors.
`make check_format` - will check all the files that differ from `master`
`tools/check_code_style.sh file name` will check a file
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725573
##########
File path: tools/Makefile.unix
##########
@@ -192,11 +192,18 @@ NUTTXNAME = nuttx
BIN = $(NUTTXNAME)$(EXEEXT)
all: $(BIN)
-.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean
+.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean check_format
ifeq ($(GIT_DIR),y)
Review comment:
For all of these.. same changes should be applied to Makefile.win. No is using Makefile.win now, but we don't want it to get too far out of sync.
If is does not involve bash commands, the change is a simple copy-paste. Bash and core-utils commands would need to be replaced with the CMD.com equivalent.
----------------------------------------------------------------
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 #2: Master pr nxstyle
improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568538098
Is it that you both are too close to the solution space that you cannot see
this from a N00B perspective?
Do you have tab completion on make installed?
Assume want to grow the NuttX community.
Assume the first question is "What is make?"
One interface in a dev environment is the "user" interface at any skill
level will further the effort and it does not hinder the "pros"
*From:* patacongo [mailto:notifications@github.com]
*Sent:* Monday, December 23, 2019 9:07 AM
*To:* apache/incubator-nuttx
*Cc:* David Sidrane; Mention
*Subject:* Re: [apache/incubator-nuttx] Master pr nxstyle improvments (#2)
This will build all of the tools including a dozen or so that you don't
need and probably will never need:
$(MAKE) -C tools -f Makefile.host
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/apache/incubator-nuttx/pull/2?email_source=notifications&email_token=AAO3BXJGBRNMQLGIMAEH5ZTQ2DV4ZA5CNFSM4J6NHFG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHRQ5OI#issuecomment-568528569>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAO3BXM6LVG73WOULJ6ODXTQ2DV4ZANCNFSM4J6NHFGQ>
.
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568537855
Sorry about the Open/Close/Open/Close. Not exactly sure what I am doing. I suspect that it is the "Close and Comment" button next to the "Comment" button. I am used to hitting a "Cancel" button at that position.
Grrr.. still says closed. Clicking open the pull requests menu page does NOT seem to do it.
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725443
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
+ INFO,
+ WARN,
+ ERROR,
+ FATAL
+};
+
+const char *class_text[] =
+{
+ "info",
+ "warning",
+ "error",
+ "fatal"
+};
+
+enum file_e {
Review comment:
```suggestion
enum file_e
{
```
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360730444
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
Review comment:
Unresolved again: I still don't see the change
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360887522
##########
File path: tools/Makefile.unix
##########
@@ -324,6 +324,14 @@ tools/cnvwindeps$(HOSTEXEEXT):
# $(Q) echo "No .config file found, creating one"
# $(Q) tools/initialconfig$(HOSTEXEEXT)
+# make it easy to see what is happening
+COLOR_BLUE = \033[0;94m
Review comment:
```suggestion
COLOR_BLUE = \033[0;94m
```
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360887055
##########
File path: tools/Makefile.unix
##########
@@ -324,6 +324,14 @@ tools/cnvwindeps$(HOSTEXEEXT):
# $(Q) echo "No .config file found, creating one"
# $(Q) tools/initialconfig$(HOSTEXEEXT)
+# make it easy to see what is happening
+COLOR_BLUE = \033[0;94m
Review comment:
There is no coding standard governing Makefiles. Other locations in this file do put spaces after comments, just like the C requirement.
----------------------------------------------------------------
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] patacongo closed pull request #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo closed pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2
----------------------------------------------------------------
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 #2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360904297
##########
File path: tools/Makefile.unix
##########
@@ -324,6 +324,14 @@ tools/cnvwindeps$(HOSTEXEEXT):
# $(Q) echo "No .config file found, creating one"
# $(Q) tools/initialconfig$(HOSTEXEEXT)
+# make it easy to see what is happening
+COLOR_BLUE = \033[0;94m
Review comment:
@davids5 I would prefer we work on check_code_style.sh first, and forget Makefile.unix at all.
Why? this avoid we couple the script logic with Makefle, for example:
1.The above implementation make the standalone script can't colorize the output.
2.The dependence rule can't work if user invoke the script from shell directly.
The best aproach is that we focus on the script development and make it work from command line as we want, then we can integrate the script either as the make target or precommit hook. Otherwise, the script will do a major rework for the standalone case.
Invoking the script from command line is the key feature for this tool, becasue we need integrate this tool the various workflow(e.g. Makefile, precommit hook, jenkins job).
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725965
##########
File path: tools/nxstyle.c
##########
@@ -477,7 +543,7 @@ int main(int argc, char **argv, char **envp)
(strcmp(line, " * Private Functions\n") == 0 ||
strcmp(line, " * Public Functions\n") == 0))
{
- //fprintf(stderr, "Functions begin at line %d:%d\n", lineno, n);
+ //ERROR("Functions begin", lineno, n);
Review comment:
```suggestion
#if 0 /* Ask Greg why it was commented out */
ERROR("Functions begin", lineno, n);
#endif
```
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725490
##########
File path: tools/nxstyle.c
##########
@@ -477,7 +543,7 @@ int main(int argc, char **argv, char **envp)
(strcmp(line, " * Private Functions\n") == 0 ||
strcmp(line, " * Public Functions\n") == 0))
{
- //fprintf(stderr, "Functions begin at line %d:%d\n", lineno, n);
+ //ERROR("Functions begin", lineno, n);
bfunctions = true;
Review comment:
Same as above: Remove or use #if 0 with comment. No C++ // comments. I see the origin code had // too. My bad, but let's fix it.
By there way, this is not an error. If just means that the tool found where function data begins following all of the data and type declarations.
----------------------------------------------------------------
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 issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568378194
@btashton @davids5 What I mean here is that we don't need modify Makefile, is engouh to add a script invoke nxstyle(maybe build it too) to check the style for specific file/files or commit/commits.
Then we instruct people integrate this script into pre-commit hook to enable the check automatically. Like:
https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl
Nobody try to integrate checkpatch.pl into Linux build system, since developer can run checkpatch.pl directly to:
1.The bunch of specified files
2.The bunch of specified commits
3.The current living change
4.Integrate to pre-commit hook
It's much flexible than make check_format since we can pass the different argument to script form command line, but the same thing is hard to do for Makefile.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360726623
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
Review comment:
There is a missing dependency here. tools/nxstyle[.exe] is not build by default but must be explictly built. Can you add
`nxstyle$(EXEEXT):
$(MAKE) -C tools -f Makfile.host nxstyle$(EXEEXT)`
check_format: nxstyle$(EXEEXT)
...
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568540319
I don't think Xaio Xiang's proposal and your implementation are mutually exclusive.
Correct me if I am misreading things, but I believe that the situation here is that David serves two masters, primarily PX4 and secondarily NuttX. So his objective will always be to submit the optimal PX4 solution and then attempt to bulldoze it through, regardless of what we feel about it. I presume that is why he is so opposed to having high level workflow requirements because they may conflict with PX4.
The may be a good thing, but one side effect that that whatever you may think will probably be ignored. The PX4 code is not open to compromise for NuttX. We will just have to see how all of this works out. Be careful but be open minded.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360905760
##########
File path: tools/Makefile.unix
##########
@@ -324,6 +324,14 @@ tools/cnvwindeps$(HOSTEXEEXT):
# $(Q) echo "No .config file found, creating one"
# $(Q) tools/initialconfig$(HOSTEXEEXT)
+# make it easy to see what is happening
+COLOR_BLUE = \033[0;94m
Review comment:
That actually makes a lot of sense, is simpler, and sounds more flexible. I would tend to support Xiao Xiangs argument.
----------------------------------------------------------------
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] patacongo commented on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568537008
> But it's very helpful to issue the above command in the script, because many user even expert don't know this trick.
There are also lots of cases where scripts in tools/ build the binaries that the scipt needs. This makes the scripts nicely self-contained.
Examples... Logic assocated with:
tools/mkconfigvars.sh:KCONFIG2MAKEFILE=Makefile.host
tools/refresh.sh:CMPCONFIGMAKEFILE=Makefile.host
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725817
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
Review comment:
```suggestion
enum class_e
{
```
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725352
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
+ INFO,
+ WARN,
+ ERROR,
+ FATAL
+};
+
+const char *class_text[] =
+{
+ "info",
+ "warning",
+ "error",
+ "fatal"
+};
+
+enum file_e {
+ unknown,
Review comment:
{ belongs on a separate line.
----------------------------------------------------------------
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 #2: Master pr nxstyle
improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568502128
>@davids5 I would prefer we work on check_code_style.sh first, and forget Makefile.unix at all.
Why? this avoid we couple the script logic with Makefle, for example:
1.The above implementation make the standalone script can't colorize the output.
2.The dependence rule can't work if user invoke the script from shell directly.
The best aproach is that we focus on the script development and make it work from command line as we want, then we can integrate the script either as the make target or precommit hook. Otherwise, the script will do a major rework for the standalone case.
Invoking the script from command line is the key feature for this tool, becasue we need integrate this tool the various workflow(e.g. Makefile, precommit hook, jenkins job).
@xiaoxiang781216 - This is now 2 simple so you can separate them and do as you think best.
Please just release something that can be used with eclipse and VSCODE - The old tool wasted way too much time. In the mean time I will backport this to PX4 as [REJECTED] to keep our users productive.
----------------------------------------------------------------
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 #2: Master pr nxstyle
improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568525036
>Yes, but we already have the rule in tools/Makefile.host for generating nxstyle binary, the question is where should we trigger the target. It's better to let the script trigger the buildotherwise user can't invoke the tool outside the make environment(e.g. from jenkins job, precommit hook or command line).
Why is the building of the host tools not part of the top level make process? Shouldn't the host tools be built So that there is a sane environment prior to building the config target?
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360730399
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
Review comment:
Description of all new make targets should be included in the top-level README, including a description of what the make target does.
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360804370
##########
File path: tools/nxstyle.c
##########
@@ -140,7 +207,7 @@ int main(int argc, char **argv, char **envp)
maxline = 78;
filename = argv[1];
-
+ g_file_name = filename;
/* Usage: nxstyle [-m <maxline>] <filename>
Review comment:
```suggestion
/* Usage: nxstyle [-m <maxline>] <filename>
```
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360727398
##########
File path: tools/check_code_style.sh
##########
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
+if [ -z "$1" ]; then
+ FILES=$(git diff master --name-only);
Review comment:
Similar problem here. You must check if .git/ exists. If it does not, you may not use any git commands. Most users do not have .git/ and this will break for the majority of users.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360887345
##########
File path: tools/check_code_style.sh
##########
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
+if [ -z "$1" ]; then
+ FILES=$(git diff master --name-only);
Review comment:
With no workflow requirements it is not safe to assume anything.
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360866861
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
Review comment:
```
# This is a top-level "kludge" Makefile that just includes the correct
# Makefile. If you already know the Makefile that you want, you can skip
# this nonsense using:
#
# make -f Makefile.unix, OR
# make -f Makefile.win
#
-include .config
ifeq ($(CONFIG_WINDOWS_NATIVE),y)
include tools/Makefile.win
else
include tools/Makefile.unix
endif
```
@patacongo Here?
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360726623
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
Review comment:
There is a missing dependency here. tools/nxstyle[.exe] is not build by default but must be explictly built. Can you add
`tools/nxstyle$(EXEEXT):
$(MAKE) -C tools -f Makfile.host nxstyle$(EXEEXT)
check_format: tools/nxstyle$(EXEEXT)
...`
----------------------------------------------------------------
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] patacongo commented on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568604222
I would like to close this now. I do not think it should be incorporated into the NuttX repositories now. It deals with specific logic for the NuttX workflow. We need to have a defintiion for the workflow before we can consider including it.
Then we need to bring all of the other people on board who are tasked with working with the NuttX: Haitao Liu and Anthony Merlino. A slam dunk of PX4 workflow is not acceptable.
----------------------------------------------------------------
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] patacongo edited a comment on issue #2: Master pr
nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#issuecomment-568309238
I added Xiao Xiang to the reviewers. We need to have a lot of concurrence here since this PR implements a portion of the workflow while there no workflow requirements in place. I think this piece of the workflow is not so controversial, but I would like to have Xiang provide his input as well. Xiang is the most authorative person on the build system and must be in the loop on build system changes.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360727425
##########
File path: tools/check_code_style.sh
##########
@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
+if [ -z "$1" ]; then
+ FILES=$(git diff master --name-only);
+else
+ FILES=$1
+fi
+
+DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
+for FILE in $FILES; do
+ FILENAME=$(basename $FILE)
+ FILE_EXT="${FILENAME#*.}"
+ if [ -f "$FILE" ]; then
+ if [ "$FILE_EXT" = "c" ] || [ "$FILE_EXT" = "h" ]; then
+ CHECK_FAILED=$(${DIR}/nxstyle $FILE)
+ # if [ -n "$CHECK_FAILED" ]; then
+ # ${DIR}/fix_code_style.sh --quiet < $FILE > $FILE.pretty
+ #
+ # echo
+ # git --no-pager diff --no-index --minimal --histogram --color=always $FILE $FILE.pretty
+ # rm -f $FILE.pretty
Review comment:
Again, check for the existence of .git/ before using any git commands.
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725633
##########
File path: tools/Makefile.unix
##########
@@ -192,11 +192,18 @@ NUTTXNAME = nuttx
BIN = $(NUTTXNAME)$(EXEEXT)
all: $(BIN)
-.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean
+.PHONY: dirlinks context clean_context check_context config oldconfig menuconfig nconfig qconfig gconfig export subdir_clean clean subdir_distclean distclean apps_clean apps_distclean check_format
ifeq ($(GIT_DIR),y)
Review comment:
@patacongo - did you see where I asked you for help on the Makefile Side? Please do a PR to this branch.
----------------------------------------------------------------
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] patacongo commented on a change in pull request
#2: Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360727266
##########
File path: tools/Makefile.unix
##########
@@ -697,3 +704,8 @@ apps_distclean:
ifneq ($(APPDIR),)
$(Q) $(MAKE) -C "$(TOPDIR)/$(APPDIR)" TOPDIR="$(TOPDIR)" distclean
endif
+
+check_format:
+ $(call colorecho,'Checking NuttX formatting with nxstyle')
+ @$(TOPDIR)/tools/check_code_style.sh
+ @cd "$(SRC_DIR)" && git diff --check
Review comment:
There is another problem here: You must account for that fact that .git/ is NOT present in most people's environment. Please make the last line conditioned on #ifeq ($(GIT_DIR),y) or you will break 80% of the nuttx user's builds!
----------------------------------------------------------------
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 #2:
Master pr nxstyle improvments
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2: Master pr nxstyle improvments
URL: https://github.com/apache/incubator-nuttx/pull/2#discussion_r360725846
##########
File path: tools/nxstyle.c
##########
@@ -50,6 +50,45 @@
#define LINE_SIZE 512
+#define FATAL(m) message(FATAL, (m), 0, 0)
+#define FATALFL(m,s) message(FATAL, (m), -1, -1)
+#define WARN(m, l, o) message(WARN, (m), (l), (0))
+#define ERROR(m, l, o) message(ERROR, (m), (l), (0))
+#define INFO(m, l, o) message(INFO, (m), (l), (0))
+
+/****************************************************************************
+ * Private types
+ ****************************************************************************/
+
+enum class_e {
+ INFO,
+ WARN,
+ ERROR,
+ FATAL
+};
+
+const char *class_text[] =
+{
+ "info",
+ "warning",
+ "error",
+ "fatal"
+};
+
+enum file_e {
+ unknown,
+ c_header,
+ c_source,
+};
+
+/****************************************************************************
+ * Private data
+ ****************************************************************************/
+
+int g_status = 0;
Review comment:
```suggestion
static int g_status = 0;
```
----------------------------------------------------------------
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