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/05/29 21:54:42 UTC
[GitHub] [incubator-nuttx] patacongo opened a new pull request #1149: tools/: Hook incdir.c into build system.
patacongo opened a new pull request #1149:
URL: https://github.com/apache/incubator-nuttx/pull/1149
## Summary
1. incdir.c was added in PR 1148. This PR hooks it into the build system.
2. tools/Makefile.host: Add incdir binary to Makefile.host. This was missed in PR 1148
## Impact
This has the potential to break builds. Significant testing has been and is being performed on individual builds. Running the change through the PR checks will be the best, final test.
## Testing
The PR checks for this PR is the final verification of this 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1149: tools/: Hook incdir.c into build system.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1149:
URL: https://github.com/apache/incubator-nuttx/pull/1149#discussion_r432809102
##########
File path: tools/Makefile.unix
##########
@@ -381,9 +370,9 @@ include tools/LibTargets.mk
# is an archive. Exactly what is performed during pass1 or what it generates
# is unknown to this makefile unless CONFIG_PASS1_OBJECT is defined.
-pass1: $(USERLIBS)
+pass1: tools/incdir$(HOSTEXEEXT) $(USERLIBS)
Review comment:
This change will break the concurrent build since tools/incdir and libraries in $(USERLIBS) and $(NUTTxLIBS) don't have the dependency and will build at the same 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
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #1149: tools/: Hook incdir.c into build system.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #1149:
URL: https://github.com/apache/incubator-nuttx/pull/1149#discussion_r432953170
##########
File path: tools/Makefile.unix
##########
@@ -381,9 +370,9 @@ include tools/LibTargets.mk
# is an archive. Exactly what is performed during pass1 or what it generates
# is unknown to this makefile unless CONFIG_PASS1_OBJECT is defined.
-pass1: $(USERLIBS)
+pass1: tools/incdir$(HOSTEXEEXT) $(USERLIBS)
Review comment:
#1152 removes this dependency, so nothing 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1149: tools/: Hook incdir.c into build system.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1149:
URL: https://github.com/apache/incubator-nuttx/pull/1149#discussion_r432846024
##########
File path: tools/Makefile.unix
##########
@@ -381,9 +370,9 @@ include tools/LibTargets.mk
# is an archive. Exactly what is performed during pass1 or what it generates
# is unknown to this makefile unless CONFIG_PASS1_OBJECT is defined.
-pass1: $(USERLIBS)
+pass1: tools/incdir$(HOSTEXEEXT) $(USERLIBS)
Review comment:
This is a race condition doesn't always happen, just like the mulitple thread issue.
----------------------------------------------------------------
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 #1149: tools/: Hook incdir.c into build system.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #1149:
URL: https://github.com/apache/incubator-nuttx/pull/1149#discussion_r432833534
##########
File path: tools/Makefile.unix
##########
@@ -381,9 +370,9 @@ include tools/LibTargets.mk
# is an archive. Exactly what is performed during pass1 or what it generates
# is unknown to this makefile unless CONFIG_PASS1_OBJECT is defined.
-pass1: $(USERLIBS)
+pass1: tools/incdir$(HOSTEXEEXT) $(USERLIBS)
Review comment:
I was concerned about this part as well, but it looks like the nightly build went well for today.
----------------------------------------------------------------
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 #1149: tools/: Hook incdir.c into build system.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #1149:
URL: https://github.com/apache/incubator-nuttx/pull/1149#discussion_r432872103
##########
File path: tools/Makefile.unix
##########
@@ -381,9 +370,9 @@ include tools/LibTargets.mk
# is an archive. Exactly what is performed during pass1 or what it generates
# is unknown to this makefile unless CONFIG_PASS1_OBJECT is defined.
-pass1: $(USERLIBS)
+pass1: tools/incdir$(HOSTEXEEXT) $(USERLIBS)
Review comment:
Yes, I understand that.
What do you think about what's described here: https://stackoverflow.com/questions/16829933/how-can-i-use-notparallel-in-makefile-only-on-specific-targets
In our example it could be something like:
```Make
pass1libs: $(USERLIBS)
pass1:
$(MAKE) tools/incdir$(HOSTEXEEXT)
$(MAKE) pass1libs
pass2libs: $(NUTTXLIBS)
pass2:
$(MAKE) tools/incdir$(HOSTEXEEXT)
$(MAKE) pass2libs
```
Or even
```Make
pass1libs: $(USERLIBS)
pass1: tools/incdir$(HOSTEXEEXT)
$(MAKE) pass1libs
pass2libs: $(NUTTXLIBS)
pass2: tools/incdir$(HOSTEXEEXT)
$(MAKE) pass2libs
```
I guess also the order-only dependency answer could work (if acceptable in our Makefiles):
```Make
pass1: $(USERLIBS) | tools/incdir$(HOSTEXEEXT)
pass2: $(NUTTXLIBS) | tools/incdir$(HOSTEXEEXT)
```
This would insure that the libs are not built before incdir (but will not update pass1/2 if incdir is changed)
----------------------------------------------------------------
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 merged pull request #1149: tools/: Hook incdir.c into build system.
Posted by GitBox <gi...@apache.org>.
Ouss4 merged pull request #1149:
URL: https://github.com/apache/incubator-nuttx/pull/1149
----------------------------------------------------------------
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] patacongo commented on pull request #1149: tools/: Hook incdir.c into build system.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1149:
URL: https://github.com/apache/incubator-nuttx/pull/1149#issuecomment-636243714
> Let's get this one in and see if there is any issue with the nightly build.
@xiaoxiang781216 @yamt Please continue provide review comments. I will address all review comments after the merge with another PR.
I am very pleased with the result of this change. I am not getting 46 seconds builds under Cygwin with -j. I have never had such great performance under cygwin before.
@Ouss4 also noted some build improvements even with Linux which I was not expecting.
I am thinking we should convert any other scripts to .c files if we think they are performance bottle necks. Not only do C files improve performance, but they can also support portability to native mode builds (I suspect that incdir.c does not have enough in place to support the native build, that is unverified). But, for example, configure.c does support Windows native 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1149: tools/: Hook incdir.c into build system.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1149:
URL: https://github.com/apache/incubator-nuttx/pull/1149#discussion_r432809102
##########
File path: tools/Makefile.unix
##########
@@ -381,9 +370,9 @@ include tools/LibTargets.mk
# is an archive. Exactly what is performed during pass1 or what it generates
# is unknown to this makefile unless CONFIG_PASS1_OBJECT is defined.
-pass1: $(USERLIBS)
+pass1: tools/incdir$(HOSTEXEEXT) $(USERLIBS)
Review comment:
This change will break the concurrent build since tools/incdir and library in $(USERLIBS) and $(NUTTxLIBS) build at the same 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1149: tools/: Hook incdir.c into build system.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1149:
URL: https://github.com/apache/incubator-nuttx/pull/1149#issuecomment-636274948
Before change:
real 0m27.361s
user 1m22.813s
sys 0m22.934s
After change:
real 0m19.003s
user 1m3.650s
sys 0m22.624s
The build is faster than before.
----------------------------------------------------------------
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 #1149: tools/: Hook incdir.c into build system.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1149:
URL: https://github.com/apache/incubator-nuttx/pull/1149#discussion_r432846024
##########
File path: tools/Makefile.unix
##########
@@ -381,9 +370,9 @@ include tools/LibTargets.mk
# is an archive. Exactly what is performed during pass1 or what it generates
# is unknown to this makefile unless CONFIG_PASS1_OBJECT is defined.
-pass1: $(USERLIBS)
+pass1: tools/incdir$(HOSTEXEEXT) $(USERLIBS)
Review comment:
The race condition doesn't always happen, just like the mulitple thread issue.
----------------------------------------------------------------
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