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 2022/01/05 21:20:35 UTC
[GitHub] [incubator-nuttx] AlanRosenthal opened a new pull request #5176: Cleanup Context/Dirlinks targets
AlanRosenthal opened a new pull request #5176:
URL: https://github.com/apache/incubator-nuttx/pull/5176
## Summary
Previously the context rule and dirlinks rule were doing the same thing.
* the `context` target called other `context` targets in subdirectories.
* the `context` targets had `.dirlinks` as a prerequisite, which created symlinks
* the `.dirlinks` target called other `.dirlinks` targets in subdirectories, which created symlinks
Changes:
* `.dirlinks` doesn't call other sub-`.dirlinks` targets. All `context` targets will take care of creating symlinks by having `.dirlinks` as prerequisites
* There was logic in `tools/Config.mk` which was duplicated in `boards/Makefile`. This PR removes the duplicated logic
* `apps_preconfig` was incorrectly depending on `.dirlinks`
* All `Configuration targets` now run the in the following order, ensuring deterministic behavior: `clean-context`, `context`, `apps_preconfig` then `kconfig...`
## Impact
This PR cleans up and isolates logic. It additionally makes the `Configuration targets` set more deterministic by running `clean-context` then `context`
## Testing
Verified build locally, CI will verify all the 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] AlanRosenthal commented on a change in pull request #5176: Cleanup Context/Dirlinks targets
Posted by GitBox <gi...@apache.org>.
AlanRosenthal commented on a change in pull request #5176:
URL: https://github.com/apache/incubator-nuttx/pull/5176#discussion_r779149800
##########
File path: tools/Unix.mk
##########
@@ -589,36 +579,43 @@ pass2dep: context tools/mkdeps$(HOSTEXEEXT) tools/cnvwindeps$(HOSTEXEEXT)
config:
$(Q) $(MAKE) clean_context
+ $(Q) $(MAKE) context
$(Q) $(MAKE) apps_preconfig
$(Q) APPSDIR=${CONFIG_APPS_DIR} EXTERNALDIR=$(EXTERNALDIR) kconfig-conf Kconfig
oldconfig:
$(Q) $(MAKE) clean_context
+ $(Q) $(MAKE) context
$(Q) $(MAKE) apps_preconfig
$(Q) APPSDIR=${CONFIG_APPS_DIR} EXTERNALDIR=$(EXTERNALDIR) kconfig-conf --oldconfig Kconfig
olddefconfig:
$(Q) $(MAKE) clean_context
+ $(Q) $(MAKE) context
$(Q) $(MAKE) apps_preconfig
$(Q) APPSDIR=${CONFIG_APPS_DIR} EXTERNALDIR=$(EXTERNALDIR) kconfig-conf --olddefconfig Kconfig
menuconfig:
$(Q) $(MAKE) clean_context
+ $(Q) $(MAKE) context
Review comment:
makefile rules are indented with tabs, and makefiles lists are intended with spaces
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] AlanRosenthal commented on pull request #5176: Cleanup Context/Dirlinks targets
Posted by GitBox <gi...@apache.org>.
AlanRosenthal commented on pull request #5176:
URL: https://github.com/apache/incubator-nuttx/pull/5176#issuecomment-1010432893
I've decided to approach this a bit different, see https://github.com/apache/incubator-nuttx/issues/5205
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5176: Cleanup Context/Dirlinks targets
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5176:
URL: https://github.com/apache/incubator-nuttx/pull/5176#discussion_r779146121
##########
File path: tools/Unix.mk
##########
@@ -589,36 +579,43 @@ pass2dep: context tools/mkdeps$(HOSTEXEEXT) tools/cnvwindeps$(HOSTEXEEXT)
config:
$(Q) $(MAKE) clean_context
+ $(Q) $(MAKE) context
$(Q) $(MAKE) apps_preconfig
$(Q) APPSDIR=${CONFIG_APPS_DIR} EXTERNALDIR=$(EXTERNALDIR) kconfig-conf Kconfig
oldconfig:
$(Q) $(MAKE) clean_context
+ $(Q) $(MAKE) context
$(Q) $(MAKE) apps_preconfig
$(Q) APPSDIR=${CONFIG_APPS_DIR} EXTERNALDIR=$(EXTERNALDIR) kconfig-conf --oldconfig Kconfig
olddefconfig:
$(Q) $(MAKE) clean_context
+ $(Q) $(MAKE) context
$(Q) $(MAKE) apps_preconfig
$(Q) APPSDIR=${CONFIG_APPS_DIR} EXTERNALDIR=$(EXTERNALDIR) kconfig-conf --olddefconfig Kconfig
menuconfig:
$(Q) $(MAKE) clean_context
+ $(Q) $(MAKE) context
Review comment:
Why do we have different indentation here? (Same in other places)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #5176: Cleanup Context/Dirlinks targets
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5176:
URL: https://github.com/apache/incubator-nuttx/pull/5176#issuecomment-1006157092
@AlanRosenthal The custom-board logic refactoring looks good, however, did you test with a custom out-of-tree board?
I don't expect any issues but our CI only tests in-tree boards and this logic had its fair amount of breakage in the past.
I can test tomorrow with this PR if you don't have a setup ready.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] AlanRosenthal commented on a change in pull request #5176: Cleanup Context/Dirlinks targets
Posted by GitBox <gi...@apache.org>.
AlanRosenthal commented on a change in pull request #5176:
URL: https://github.com/apache/incubator-nuttx/pull/5176#discussion_r779144686
##########
File path: boards/Makefile
##########
@@ -94,19 +74,12 @@ makedepfile: $(CSRCS:.c=.ddc) $(ASRCS:.S=.dds) $(CXXSRCS:.cxx=.ddx)
depend: .depend
-$(DUMMY_KCONFIG): $(BOARD_KCONFIG)
- $(call DELFILE, $(DUMMY_KCONFIG))
- $(call COPYFILE, $(BOARD_KCONFIG), $(DUMMY_KCONFIG))
-
-dirlinks: $(DUMMY_KCONFIG)
-
-context: $(DUMMY_KCONFIG)
Review comment:
`$(DUMMY_KCONFIG)` is now built by `Unix.mk`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] AlanRosenthal commented on a change in pull request #5176: Cleanup Context/Dirlinks targets
Posted by GitBox <gi...@apache.org>.
AlanRosenthal commented on a change in pull request #5176:
URL: https://github.com/apache/incubator-nuttx/pull/5176#discussion_r779162088
##########
File path: tools/Unix.mk
##########
@@ -328,13 +328,20 @@ arch/dummy/Kconfig:
@echo "CP: $@ to $(CHIP_KCONFIG)"
$(Q) cp -f $(CHIP_KCONFIG) $@
+# Copy $(BOARD_KCONFIG) to boards/dummy/Kconfig
+
+boards/dummy/Kconfig:
+ @echo "CP: $@ to $(BOARD_KCONFIG)"
+ $(Q) cp -f $(BOARD_KCONFIG) $@
Review comment:
will do, looks like there's some issues with CI, once I have CI passing I'll mirror the changes to Win.mk
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5176: Cleanup Context/Dirlinks targets
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5176:
URL: https://github.com/apache/incubator-nuttx/pull/5176#discussion_r779149945
##########
File path: tools/Unix.mk
##########
@@ -589,36 +579,43 @@ pass2dep: context tools/mkdeps$(HOSTEXEEXT) tools/cnvwindeps$(HOSTEXEEXT)
config:
$(Q) $(MAKE) clean_context
+ $(Q) $(MAKE) context
$(Q) $(MAKE) apps_preconfig
$(Q) APPSDIR=${CONFIG_APPS_DIR} EXTERNALDIR=$(EXTERNALDIR) kconfig-conf Kconfig
oldconfig:
$(Q) $(MAKE) clean_context
+ $(Q) $(MAKE) context
$(Q) $(MAKE) apps_preconfig
$(Q) APPSDIR=${CONFIG_APPS_DIR} EXTERNALDIR=$(EXTERNALDIR) kconfig-conf --oldconfig Kconfig
olddefconfig:
$(Q) $(MAKE) clean_context
+ $(Q) $(MAKE) context
$(Q) $(MAKE) apps_preconfig
$(Q) APPSDIR=${CONFIG_APPS_DIR} EXTERNALDIR=$(EXTERNALDIR) kconfig-conf --olddefconfig Kconfig
menuconfig:
$(Q) $(MAKE) clean_context
+ $(Q) $(MAKE) context
Review comment:
Probably this is a bug of GitHub mobile app. The file view seems not having such display 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5176: Cleanup Context/Dirlinks targets
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5176:
URL: https://github.com/apache/incubator-nuttx/pull/5176#issuecomment-1006240215
@AlanRosenthal do you notice this warning from https://github.com/apache/incubator-nuttx/runs/4716428060?check_suite_focus=true:
```
Makefile:326: warning: overriding recipe for target 'config.h'
Makefile:199: warning: ignoring old recipe for target 'config.h'
Makefile:326: warning: overriding recipe for target 'config.h'
Makefile:199: warning: ignoring old recipe for target 'config.h'
Makefile:326: warning: overriding recipe for target 'config.h'
Makefile:199: warning: ignoring old recipe for target 'config.h'
Makefile:326: warning: overriding recipe for target 'config.h'
Makefile:199: warning: ignoring old recipe for target 'config.h'
Makefile:326: warning: overriding recipe for target 'config.h'
Makefile:199: warning: ignoring old recipe for target 'config.h'
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5176: Cleanup Context/Dirlinks targets
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5176:
URL: https://github.com/apache/incubator-nuttx/pull/5176#discussion_r779159668
##########
File path: tools/Unix.mk
##########
@@ -328,13 +328,20 @@ arch/dummy/Kconfig:
@echo "CP: $@ to $(CHIP_KCONFIG)"
$(Q) cp -f $(CHIP_KCONFIG) $@
+# Copy $(BOARD_KCONFIG) to boards/dummy/Kconfig
+
+boards/dummy/Kconfig:
+ @echo "CP: $@ to $(BOARD_KCONFIG)"
+ $(Q) cp -f $(BOARD_KCONFIG) $@
Review comment:
@AlanRosenthal, please don't forget to mirror these changes to the Win.mk 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] AlanRosenthal closed pull request #5176: Cleanup Context/Dirlinks targets
Posted by GitBox <gi...@apache.org>.
AlanRosenthal closed pull request #5176:
URL: https://github.com/apache/incubator-nuttx/pull/5176
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org