You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/08/20 17:48:14 UTC
[GitHub] [incubator-nuttx] Ouss4 opened a new pull request #1616: Align the two configure programs (configure.sh and configure.c)
Ouss4 opened a new pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616
## Summary
With #1614 when using the tool with -E and no configuration was already in place, the general "NuttX has not been configured" from the toplevel Makefile is printed before configuring.
Also apply the same changes to configure.c
## Impact
## Testing
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#issuecomment-678565445
> but nxstyle warning need be fixed first.
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
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#issuecomment-677886680
> My only concern is about whether the existence of the .config file is really a requirement for executing the [-E] command.
> IMO, if the -E is an enforcement, it should always execute the distclean regardless of any condition.
If .config doesn't exist it means that distclean is not necessary, so -E won't have any effect (no other make target will have an effect if .config is not present). This should work exactly as the last PR with only removing the unnecessary output when the project is not configured.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] ghn-certi edited a comment on pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
ghn-certi edited a comment on pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#issuecomment-677838853
> @ghn-certi just FYI.
My only concern is about whether the existence of the .config file is really a requirement for executing the [-E] command.
IMO, if the -E is an enforcement, it should always execute the distclean regardless of any condition.
Other than that, the PR looks good to me.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] ghn-certi commented on pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#issuecomment-677838853
> @ghn-certi just FYI.
My only concern was about whether the existence of the .config file is a requirement executing the [-E] command.
Again, if the -E is an enforcement, it should always execute regardless of any condition.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#issuecomment-678324024
LGTM, but nxstyle warning need be fixed first.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] ghn-certi edited a comment on pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
ghn-certi edited a comment on pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#issuecomment-677838853
> @ghn-certi just FYI.
My only concern was about whether the existence of the .config file is really a requirement executing the [-E] command.
Again, if the -E is an enforcement, it should always execute regardless of any condition.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] ghn-certi edited a comment on pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
ghn-certi edited a comment on pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#issuecomment-677838853
> @ghn-certi just FYI.
My only concern is about whether the existence of the .config file is really a requirement for executing the [-E] command.
Again, if the -E is an enforcement, it should always execute regardless of any condition.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#discussion_r474740592
##########
File path: tools/configure.c
##########
@@ -794,6 +835,8 @@ static void check_configdir(void)
static void check_configured(void)
{
+ char *defcfgpath = NULL;
Review comment:
Does coding standard require put all variables to the beginning of function? I suppose puting variables to the beginning of block is also 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#discussion_r474697965
##########
File path: tools/configure.c
##########
@@ -808,12 +851,35 @@ static void check_configured(void)
}
else
{
- fprintf(stderr, "ERROR: Found %s... Already configured\n",
- g_buffer);
- fprintf(stderr, " Please 'make distclean' and try again\n");
- exit(EXIT_FAILURE);
+ snprintf(g_buffer, BUFFER_SIZE, "%s%cdefconfig",
+ g_configpath, g_delim);
+ defcfgpath = strdup(g_buffer);
+
+ snprintf(g_buffer, BUFFER_SIZE, "%s%cdefconfig",
+ g_topdir, g_delim);
+
+ if (filecmp(g_buffer, defcfgpath))
+ {
+ fprintf(stderr, "No configuration change.\n");
+ free(defcfgpath);
+ exit(EXIT_FAILURE);
Review comment:
should we exit 0 here like configure.sh?
##########
File path: tools/configure.c
##########
@@ -794,6 +835,8 @@ static void check_configdir(void)
static void check_configured(void)
{
+ char *defcfgpath = NULL;
Review comment:
Should we move into else block at line 852 to limit the variable scope?
##########
File path: tools/configure.c
##########
@@ -808,12 +851,35 @@ static void check_configured(void)
}
else
{
- fprintf(stderr, "ERROR: Found %s... Already configured\n",
- g_buffer);
- fprintf(stderr, " Please 'make distclean' and try again\n");
- exit(EXIT_FAILURE);
+ snprintf(g_buffer, BUFFER_SIZE, "%s%cdefconfig",
+ g_configpath, g_delim);
+ defcfgpath = strdup(g_buffer);
+
+ snprintf(g_buffer, BUFFER_SIZE, "%s%cdefconfig",
+ g_topdir, g_delim);
+
+ if (filecmp(g_buffer, defcfgpath))
+ {
+ fprintf(stderr, "No configuration change.\n");
+ free(defcfgpath);
+ exit(EXIT_FAILURE);
+ }
+
+ if (g_distclean)
+ {
+ run_make("distclean");
+ }
+ else
+ {
+ fprintf(stderr, "Already configured!\n");
+ fprintf(stderr, "Please 'make distclean' and try again.\n");
+ free(defcfgpath);
+ exit(EXIT_FAILURE);
+ }
}
}
+
+ free(defcfgpath);
Review comment:
Move to line 868 and remove line 876?
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#discussion_r474743103
##########
File path: tools/configure.c
##########
@@ -808,12 +851,35 @@ static void check_configured(void)
}
else
{
- fprintf(stderr, "ERROR: Found %s... Already configured\n",
- g_buffer);
- fprintf(stderr, " Please 'make distclean' and try again\n");
- exit(EXIT_FAILURE);
+ snprintf(g_buffer, BUFFER_SIZE, "%s%cdefconfig",
+ g_configpath, g_delim);
+ defcfgpath = strdup(g_buffer);
+
+ snprintf(g_buffer, BUFFER_SIZE, "%s%cdefconfig",
+ g_topdir, g_delim);
+
+ if (filecmp(g_buffer, defcfgpath))
+ {
+ fprintf(stderr, "No configuration change.\n");
+ free(defcfgpath);
+ exit(EXIT_FAILURE);
+ }
+
+ if (g_distclean)
+ {
+ run_make("distclean");
+ }
+ else
+ {
+ fprintf(stderr, "Already configured!\n");
+ fprintf(stderr, "Please 'make distclean' and try again.\n");
+ free(defcfgpath);
+ exit(EXIT_FAILURE);
+ }
}
}
+
+ free(defcfgpath);
Review comment:
I re-organized a little here to limit the frees. Please take a look.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] ghn-certi edited a comment on pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
ghn-certi edited a comment on pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#issuecomment-677838853
> @ghn-certi just FYI.
My only concern is about whether the existence of the .config file is really a requirement executing the [-E] command.
Again, if the -E is an enforcement, it should always execute regardless of any condition.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] ghn-certi edited a comment on pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
ghn-certi edited a comment on pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#issuecomment-677838853
> @ghn-certi just FYI.
My only concern is about whether the existence of the .config file is really a requirement for executing the [-E] command.
IMO, if the -E is an enforcement, it should always execute regardless of any condition.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] ghn-certi edited a comment on pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
ghn-certi edited a comment on pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#issuecomment-677838853
> @ghn-certi just FYI.
My only concern is about whether the existence of the .config file is really a requirement for executing the [-E] command.
IMO, if the -E is an enforcement, it should always execute the distclean regardless of any condition.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#issuecomment-677808133
@ghn-certi just FYI.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#discussion_r474735419
##########
File path: tools/configure.c
##########
@@ -808,12 +851,35 @@ static void check_configured(void)
}
else
{
- fprintf(stderr, "ERROR: Found %s... Already configured\n",
- g_buffer);
- fprintf(stderr, " Please 'make distclean' and try again\n");
- exit(EXIT_FAILURE);
+ snprintf(g_buffer, BUFFER_SIZE, "%s%cdefconfig",
+ g_configpath, g_delim);
+ defcfgpath = strdup(g_buffer);
+
+ snprintf(g_buffer, BUFFER_SIZE, "%s%cdefconfig",
+ g_topdir, g_delim);
+
+ if (filecmp(g_buffer, defcfgpath))
+ {
+ fprintf(stderr, "No configuration change.\n");
+ free(defcfgpath);
+ exit(EXIT_FAILURE);
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
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616#discussion_r474737098
##########
File path: tools/configure.c
##########
@@ -794,6 +835,8 @@ static void check_configdir(void)
static void check_configured(void)
{
+ char *defcfgpath = NULL;
Review comment:
I originally put it there because of the coding standard, but these host programs don't follow that, so I moved it as suggested.
----------------------------------------------------------------
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
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #1616: Align the two configure programs (configure.sh and configure.c)
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #1616:
URL: https://github.com/apache/incubator-nuttx/pull/1616
----------------------------------------------------------------
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