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/09/15 07:05:40 UTC
[GitHub] [incubator-nuttx-apps] GUIDINGLI opened a new pull request #385: apps/MAKE: Add register staging to context
GUIDINGLI opened a new pull request #385:
URL: https://github.com/apache/incubator-nuttx-apps/pull/385
## Summary
apps/MAKE: Add register staging to context
## Impact
stage context -> stage context
stage register
## 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-apps] v01d commented on pull request #385: apps/MAKE: Add register staging to context
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #385:
URL: https://github.com/apache/incubator-nuttx-apps/pull/385#issuecomment-692721251
What is the purpose of this change? Please be more descriptive in 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
[GitHub] [incubator-nuttx-apps] v01d edited a comment on pull request #385: apps/MAKE: Add register staging to context
Posted by GitBox <gi...@apache.org>.
v01d edited a comment on pull request #385:
URL: https://github.com/apache/incubator-nuttx-apps/pull/385#issuecomment-693378138
FYI, this is the PR: https://github.com/apache/incubator-nuttx-apps/pull/388 . Once it is merged, please rebase here. You will have to remove the passing of TOPDIR and APPDIR in Make calls (but only if they are passed like TOPDIR=$(TOPDIR), if the value is different, keep 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
[GitHub] [incubator-nuttx-apps] GUIDINGLI commented on a change in pull request #385: apps/MAKE: Add register staging to context
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on a change in pull request #385:
URL: https://github.com/apache/incubator-nuttx-apps/pull/385#discussion_r489148926
##########
File path: Makefile
##########
@@ -152,13 +156,12 @@ endif # CONFIG_BUILD_KERNEL
dirlinks:
$(Q) $(MAKE) -C platform dirlinks TOPDIR="$(TOPDIR)" APPDIR="$(APPDIR)"
-context_rest: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_context)
+context_all: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_context)
+register_all: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_register)
-context_serialize:
- $(Q) $(MAKE) -C builtin context TOPDIR="$(TOPDIR)" APPDIR="$(APPDIR)"
Review comment:
the special case of running context on builtin first, is useless, so I removed.
You can see there is no "context:: " in builtin/Makefile, then it will use common context:: in Application.mk.
That is do the action of register, but builtin folder has no PROGNAME itself.
so we can remove this:
$(Q) $(MAKE) -C builtin context TOPDIR="$(TOPDIR)" APPDIR="$(APPDIR)"
----------------------------------------------------------------
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-apps] GUIDINGLI commented on pull request #385: apps/MAKE: Add register staging to context
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on pull request #385:
URL: https://github.com/apache/incubator-nuttx-apps/pull/385#issuecomment-693157298
Sure, wait for your patch merged.
----------------------------------------------------------------
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-apps] v01d commented on a change in pull request #385: apps/MAKE: Add register staging to context
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #385:
URL: https://github.com/apache/incubator-nuttx-apps/pull/385#discussion_r489441647
##########
File path: Makefile
##########
@@ -152,13 +156,12 @@ endif # CONFIG_BUILD_KERNEL
dirlinks:
$(Q) $(MAKE) -C platform dirlinks TOPDIR="$(TOPDIR)" APPDIR="$(APPDIR)"
-context_rest: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_context)
+context_all: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_context)
+register_all: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_register)
-context_serialize:
- $(Q) $(MAKE) -C builtin context TOPDIR="$(TOPDIR)" APPDIR="$(APPDIR)"
Review comment:
I'm unsure we should remove the call to a target such as context for a subdirectory because it is unused. context might someday be required in builtin and it may not be trivial to understand it is not being invoked. These targets are invoked for all subdirectories in NuttX, so let's keep it even while right now it is not used.
From what I understand $(CONFIGURED_APPS) does not contain "builtin", so unless you explicitly call it, it will not be called at all. Just add "builtin" instead the foreach of the context target. That will be enough I think.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] v01d merged pull request #385: apps/MAKE: Add register staging to context
Posted by GitBox <gi...@apache.org>.
v01d merged pull request #385:
URL: https://github.com/apache/incubator-nuttx-apps/pull/385
----------------------------------------------------------------
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-apps] v01d commented on pull request #385: apps/MAKE: Add register staging to context
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #385:
URL: https://github.com/apache/incubator-nuttx-apps/pull/385#issuecomment-693378138
FYI, this is the PR. Once it is merged, please rebase here. You will have to remove the passing of TOPDIR and APPDIR in Make calls (but only if they are passed like TOPDIR=$(TOPDIR), if the value is different, keep 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
[GitHub] [incubator-nuttx-apps] v01d commented on a change in pull request #385: apps/MAKE: Add register staging to context
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #385:
URL: https://github.com/apache/incubator-nuttx-apps/pull/385#discussion_r491439861
##########
File path: Makefile
##########
@@ -152,13 +156,12 @@ endif # CONFIG_BUILD_KERNEL
dirlinks:
$(Q) $(MAKE) -C platform dirlinks TOPDIR="$(TOPDIR)" APPDIR="$(APPDIR)"
-context_rest: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_context)
+context_all: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_context)
+register_all: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_register)
-context_serialize:
- $(Q) $(MAKE) -C builtin context TOPDIR="$(TOPDIR)" APPDIR="$(APPDIR)"
Review comment:
Ok then!
----------------------------------------------------------------
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-apps] v01d commented on pull request #385: apps/MAKE: Add register staging to context
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #385:
URL: https://github.com/apache/incubator-nuttx-apps/pull/385#issuecomment-693149779
Thanks, I now understand why the change. I think the idea is reasonable.
Maybe someone else wants to take a look and see if there isn't anything critical not considered?
BTW, if you don't mind I'd rather we wait for this to be merged https://github.com/apache/incubator-nuttx/pull/1785
since I have that PR ready and the corresponding one for apps. Adapting this PR to that is really simple, but this way
we make it in one go.
----------------------------------------------------------------
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-apps] GUIDINGLI commented on pull request #385: apps/MAKE: Add register staging to context
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on pull request #385:
URL: https://github.com/apache/incubator-nuttx-apps/pull/385#issuecomment-693142999
@v01d fill the description
----------------------------------------------------------------
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-apps] xiaoxiang781216 commented on a change in pull request #385: apps/MAKE: Add register staging to context
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #385:
URL: https://github.com/apache/incubator-nuttx-apps/pull/385#discussion_r491262420
##########
File path: Makefile
##########
@@ -152,13 +156,12 @@ endif # CONFIG_BUILD_KERNEL
dirlinks:
$(Q) $(MAKE) -C platform dirlinks TOPDIR="$(TOPDIR)" APPDIR="$(APPDIR)"
-context_rest: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_context)
+context_all: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_context)
+register_all: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_register)
-context_serialize:
- $(Q) $(MAKE) -C builtin context TOPDIR="$(TOPDIR)" APPDIR="$(APPDIR)"
Review comment:
@v01d $(CONFIGURED_APPS) actually contain "builtin", here is the code from https://github.com/apache/incubator-nuttx-apps/blob/master/builtin/Make.defs#L37:
```
ifeq ($(CONFIG_BUILTIN),y)
CONFIGURED_APPS += $(APPDIR)/builtin
endif
```
the old Makefile build the context target of builtin twice, this patch fix this issue by removing the explicit invocation.
----------------------------------------------------------------
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-apps] v01d commented on a change in pull request #385: apps/MAKE: Add register staging to context
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #385:
URL: https://github.com/apache/incubator-nuttx-apps/pull/385#discussion_r489135952
##########
File path: Makefile
##########
@@ -152,13 +156,12 @@ endif # CONFIG_BUILD_KERNEL
dirlinks:
$(Q) $(MAKE) -C platform dirlinks TOPDIR="$(TOPDIR)" APPDIR="$(APPDIR)"
-context_rest: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_context)
+context_all: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_context)
+register_all: $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_register)
-context_serialize:
- $(Q) $(MAKE) -C builtin context TOPDIR="$(TOPDIR)" APPDIR="$(APPDIR)"
Review comment:
aren't you removing the special case of running context on builtin first? I don't see where builtin is called after your 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