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/07/24 23:17:45 UTC
[GitHub] [incubator-nuttx-apps] Ouss4 opened a new pull request #341: Don't preconfig, clean or distclean when it's not necessary
Ouss4 opened a new pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341
## Summary
This PR has 2 main commits. The first one related to #340. It prevents the `preconfig` target from executing in application directories that don't actually need any pre-configuration.
The second one limits the execution scope of `clean` and `distclean` to the folder that we know we have built, instead of recursively going through every sub-directory.
## Impact
The `preconfig` commit reduces configuration time.
The `clean` and `distclean` commit reduces cleaning time.
Both remove a bunch of unnecessary output.
The whole `./tools/configure.sh make make distclean` procedure shall be faster.
## Testing
A bunch of configurations with different examples, hello, helloxx, ostest, etc.
----------------------------------------------------------------
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] btashton commented on a change in pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#discussion_r460414044
##########
File path: Directory.mk
##########
@@ -35,36 +35,30 @@
include $(APPDIR)/Make.defs
-# Sub-directories
+# Sub-directories that have been built.
-SUBDIRS = $(dir $(wildcard */Makefile))
+CLEANSUBDIRS = $(dir $(wildcard */.built))
+DISTCLEANSUBDIRS = $(dir $(wildcard */.depend))
all: nothing
-.PHONY: nothing context depend clean distclean
+.PHONY: nothing clean distclean
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),preconfig)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),context)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),depend)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),clean)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),distclean)))
+$(foreach SDIR, $(CLEANSUBDIRS), $(eval $(call SDIR_template,$(SDIR),clean)))
+$(foreach SDIR, $(DISTCLEANSUBDIRS), $(eval $(call SDIR_template,$(SDIR),distclean)))
Review comment:
@xiaoxiang781216 I was under the impression that a foreach with eval call would not be run with j parallelism
----------------------------------------------------------------
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] Ouss4 edited a comment on pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
Ouss4 edited a comment on pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#issuecomment-663776227
> I believe that the nuttx/ repository still has this problem. Objects will get left in directories if you reconfigure and CLEANDIRS changes.
I gave it a quick try by enabling a driver, building, disabling the driver then clean. It seems that its object files are removed. But that wasn't a thorough investigation.
Regarding the config that's failing, it's not related to this change. It seems that stm32f4discovery:hciuart defconfig has changed. It builds fine but refresh.sh fails.
EDIT: It actually is related to the preconfig commit. I'm trying to find a way to fix 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] xiaoxiang781216 commented on a change in pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#discussion_r460416199
##########
File path: Directory.mk
##########
@@ -35,36 +35,30 @@
include $(APPDIR)/Make.defs
-# Sub-directories
+# Sub-directories that have been built.
-SUBDIRS = $(dir $(wildcard */Makefile))
+CLEANSUBDIRS = $(dir $(wildcard */.built))
+DISTCLEANSUBDIRS = $(dir $(wildcard */.depend))
all: nothing
-.PHONY: nothing context depend clean distclean
+.PHONY: nothing clean distclean
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),preconfig)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),context)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),depend)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),clean)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),distclean)))
+$(foreach SDIR, $(CLEANSUBDIRS), $(eval $(call SDIR_template,$(SDIR),clean)))
+$(foreach SDIR, $(DISTCLEANSUBDIRS), $(eval $(call SDIR_template,$(SDIR),distclean)))
Review comment:
foreach SDIR_template with will expand to something like this:
```
xxx_clean:
make -C xxx clean
```
And the follow line:
```
clean: $(foreach SDIR, $(CLEANSUBDIRS), $(SDIR)_clean)
```
will expand to:
```
clean: xxx_clean ...
```
so make will execute xxx_clean concurrently.
----------------------------------------------------------------
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] Ouss4 commented on pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#issuecomment-663772697
> This behavior was intentional. If you do not clean ALL directories, then when you reconfigure and disable an app, the object files and other products will be stranded in the directory with no way to clean them
Yes, I was careful regarding this part.
The directories that are cleaned or distcleaned are the directories that contain those object files. So even if an app is disabled, its directory will still be cleaned during the next clean/distclean.
----------------------------------------------------------------
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 pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#issuecomment-663857050
LGTM. @patacongo @btashton could you merge the patch if no more concern.
----------------------------------------------------------------
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 #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#discussion_r460388333
##########
File path: Directory.mk
##########
@@ -35,36 +35,30 @@
include $(APPDIR)/Make.defs
-# Sub-directories
+# Sub-directories that have been built.
-SUBDIRS = $(dir $(wildcard */Makefile))
+CLEANSUBDIRS = $(dir $(wildcard */.built))
+DISTCLEANSUBDIRS = $(dir $(wildcard */.depend))
all: nothing
-.PHONY: nothing context depend clean distclean
+.PHONY: nothing clean distclean
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),preconfig)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),context)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),depend)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),clean)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),distclean)))
+$(foreach SDIR, $(CLEANSUBDIRS), $(eval $(call SDIR_template,$(SDIR),clean)))
+$(foreach SDIR, $(DISTCLEANSUBDIRS), $(eval $(call SDIR_template,$(SDIR),distclean)))
Review comment:
@btashton the current form already run parallelly.
----------------------------------------------------------------
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] patacongo commented on pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#issuecomment-663774281
> The original bad version just cleaned in CONFIGURED_APPS. That was the version that did not work because it stranded objects when apps were removed from CONFIGURED_APPS. Your logic is smarter; it looks at some landmarks to see if the directory was built.
I believe that the nuttx/ repository still has this problems. Objects will get left in directories if you configure and CLEANDIRS 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
[GitHub] [incubator-nuttx-apps] patacongo edited a comment on pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#issuecomment-663772153
> The second one limits the execution scope of `clean` and `distclean` to the folder that we know we have built, instead of recursively going through every sub-directory.
This behavior was intentional. If you do not clean ALL directories, then when you reconfigure and disable an app, the object files and other products will be stranded in the directory with no way to clean them. You should not make this change; we must continue to clean in all directories.
,,,
Hmmm... I see that your change is smarter than the original bad version. The original bad version just cleaned in CONFIGURED_APPS. That was the version that did not work because it stranded objects when apps were removed from CONFIGURED_APPS. Your logic is smarter; it looks at some landmarks to see if the directory was built.
So I retract my original comment. But I will leave to others to assure that the behavior is good on a reconfiguration.
----------------------------------------------------------------
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] patacongo commented on pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#issuecomment-663772153
> The second one limits the execution scope of `clean` and `distclean` to the folder that we know we have built, instead of recursively going through every sub-directory.
This behavior was intentional. If you do not clean ALL directories, then when you reconfigure and disable an app, the object files and other products will be stranded in the directory with no way to clean them. You should not make this change; we must continue to clean in all directories.
If this is merged there will be much breakage (it used to work this way years ago, but the behavior was wrong on a reconfiguration).
----------------------------------------------------------------
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] Ouss4 commented on pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#issuecomment-663786450
> I often saw them under boards and arch
This rings a bell. I've seen some myself too, I'll take a look after I'm done with this.
@patacongo @btashton I'm seeing some Kconfig leftovers with this PR, let me sleep on it and check tomorrow with a fresher mind.
----------------------------------------------------------------
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] Ouss4 commented on pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#issuecomment-663776227
> I believe that the nuttx/ repository still has this problem. Objects will get left in directories if you reconfigure and CLEANDIRS changes.
I gave it a quick try by enabling a driver, building, disabling the driver then clean. It seems that its object files are removed. But that wasn't a thorough investigation.
Regarding the config that's failing, it's not related to this change. It seems that stm32f4discovery:hciuart defconfig has changed. It builds fine but refresh.sh fails.
----------------------------------------------------------------
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] patacongo merged pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341
----------------------------------------------------------------
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] Ouss4 commented on pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#issuecomment-663848913
@patacongo @btashton @xiaoxiang781216 could you please give it another look? All the checks have passed.
I added a bunch of changes to fix some corner cases.
----------------------------------------------------------------
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] patacongo edited a comment on pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#issuecomment-663774281
> The original bad version just cleaned in CONFIGURED_APPS. That was the version that did not work because it stranded objects when apps were removed from CONFIGURED_APPS. Your logic is smarter; it looks at some landmarks to see if the directory was built.
I believe that the nuttx/ repository still has this problem. Objects will get left in directories if you reconfigure and CLEANDIRS 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
[GitHub] [incubator-nuttx-apps] patacongo edited a comment on pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#issuecomment-663781774
> > I believe that the nuttx/ repository still has this problem. Objects will get left in directories if you reconfigure and CLEANDIRS changes.
>
> I gave it a quick try by enabling a driver, building, disabling the driver then clean. It seems that its object files are removed. But that wasn't a thorough investigation.
I think that the CLEANDIRS only addresses top-level directories. So you would only see the issue, for example, if you built with networking, then disabled networking and rebuilt.
I have often seen leftover objects under nuttx/ and I am think that I often saw them under boards and arch, but I can't remember. Anyway, that is a different subject than this PR.
Most nuttx/ sub-directories ... including drivers/ ... are immune to left over objects because all of the objects are collected in the top-level direct rather than in sub-directories. But if you continue to re-use the fork with many difference configurations, you will eventually see stranded objects. I haven't studied why in any detail.
----------------------------------------------------------------
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] patacongo commented on pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#issuecomment-663781774
> > I believe that the nuttx/ repository still has this problem. Objects will get left in directories if you reconfigure and CLEANDIRS changes.
>
> I gave it a quick try by enabling a driver, building, disabling the driver then clean. It seems that its object files are removed. But that wasn't a thorough investigation.
I think that the CLEANDIRS only addresses top-level directories. So you would only see the issue, for example, if you built with networking, then disabled networking and rebuilt.
I have often seen leftover objects under nuttx/ and I am think that I often saw them under boards and arch, but I can't remember. Anyway, that is a different subject than this 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] btashton commented on a change in pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#discussion_r460338878
##########
File path: Directory.mk
##########
@@ -35,36 +35,30 @@
include $(APPDIR)/Make.defs
-# Sub-directories
+# Sub-directories that have been built.
-SUBDIRS = $(dir $(wildcard */Makefile))
+CLEANSUBDIRS = $(dir $(wildcard */.built))
+DISTCLEANSUBDIRS = $(dir $(wildcard */.depend))
all: nothing
-.PHONY: nothing context depend clean distclean
+.PHONY: nothing clean distclean
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),preconfig)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),context)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),depend)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),clean)))
-$(foreach SDIR, $(SUBDIRS), $(eval $(call SDIR_template,$(SDIR),distclean)))
+$(foreach SDIR, $(CLEANSUBDIRS), $(eval $(call SDIR_template,$(SDIR),clean)))
+$(foreach SDIR, $(DISTCLEANSUBDIRS), $(eval $(call SDIR_template,$(SDIR),distclean)))
Review comment:
Should these be moved to a make target so they can use parallelism of '-j'?
----------------------------------------------------------------
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] patacongo edited a comment on pull request #341: Don't preconfig, clean or distclean when it's not necessary
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #341:
URL: https://github.com/apache/incubator-nuttx-apps/pull/341#issuecomment-663774281
> The original bad version just cleaned in CONFIGURED_APPS. That was the version that did not work because it stranded objects when apps were removed from CONFIGURED_APPS. Your logic is smarter; it looks at some landmarks to see if the directory was built.
I believe that the nuttx/ repository still has this problem. Objects will get left in directories if you configure and CLEANDIRS 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