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/12 03:47:52 UTC
[GitHub] [incubator-nuttx-apps] v01d opened a new pull request #380: Fix: ensure archive files do not carry object files from prior builds
v01d opened a new pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380
## Summary
This is the corresponding change to the one on main NuttX repo (see https://github.com/apache/incubator-nuttx/pull/1765).
In this case this involves splitting the build of libapps.a into: a) building
all applications (which is safely parallelizable), b) adding each
application's object files to the archive in turns (serial by nature).
This removes the need for the flock used to protect the parallel build.
NOTE: this will not pass CI since it depends on the other PR
## Impact
Build
## Testing
Builds correctly
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487401602
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
Should we use ARCHIVE_ADD here?
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
Should we use ARCHIVE_ADD here?
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
Should we use ARCHIVE_ADD here?
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
Should we use ARCHIVE_ADD here?
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487401602
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
Should we use ARCHIVE_ADD here?
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
Should we use ARCHIVE_ADD here?
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
Should we use ARCHIVE_ADD here?
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
Should we use ARCHIVE_ADD here?
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
Should we use ARCHIVE_ADD here?
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487551931
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Right. The documentation is on nuttx repo so I will add the change there.
As per removing flock from CI this is on the testing repo. I can do that once these two PRs are 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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487412565
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Yes, it appears .depend covers it, don't really understand the purpose of the extra search for .built.
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487430223
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I managed to solve this, I force pushed the 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-apps] v01d commented on a change in pull request #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487411033
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Yes
----------------------------------------------------------------
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 a change in pull request #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487418655
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
----------------------------------------------------------------
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 a change in pull request #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487418655
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built". I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487411033
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Yes
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
I understand this is the only call to the archive being created (it is either the previous recursive call into subdirectories or this one, according to the `if`). So in this case we need to create the library from scratch. But I'm not really familiar with this SYMTABOBJ rule.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I will take a look at the clean.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Yes, it appears .depend covers it, don't really understand the purpose of the extra search for .built.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
> I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
> If ".built" is not needed, please remove the search from Direcotry.mk too.
Yes, I did the same check. I have already modified my local commit to include it but did not want to push to step on your review.
>
> BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
Thanks for testing, yes it fails. It seems this is the LOADABLE case. I will look into how to fix this.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I managed to solve this, I force pushed the change.
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
You were right, this needed ARCHIVE_ADD and remove the library earlier. I just pushed a change which consideres both the CONFIG_BUILD_LOADABLE and !CONFIG_BUILD_LOADABLE case.
----------------------------------------------------------------
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 a change in pull request #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487386070
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Hmm.. is ".depend" covering everything?
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487549682
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
Review comment:
should we change to archive to reflect what this tep really do?
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487549853
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
Review comment:
yes, sounds better
----------------------------------------------------------------
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 a change in pull request #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487390584
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Is this to remove old members that are not going to get replaced with the 'r' of `ar crs`?
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#issuecomment-691695735
LGTM, @Ouss4 please merge it if you are fine too.
----------------------------------------------------------------
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 a change in pull request #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487385182
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build in a stable situation. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Directories that are going to be cleaned are filtered-out using this marker ".built". Removing it will keep object files (and other autogenerated files) after a 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 merged pull request #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380
----------------------------------------------------------------
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 a change in pull request #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487385182
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build in a stable situation. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Directories that are going to be cleaned are filtered-out using this marker ".built". Removing it will keep object files (and other autogenerated files) after a clean/distclean.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Hmm.. is ".depend" covering everything?
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Is this to remove old members that are not going to get replaced with the 'r' of `ar crs`?
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built". I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build in a stable situation. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Directories that are going to be cleaned are filtered-out using this marker ".built". Removing it will keep object files (and other autogenerated files) after a clean/distclean.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Hmm.. is ".depend" covering everything?
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Is this to remove old members that are not going to get replaced with the 'r' of `ar crs`?
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built". I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487419635
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
> I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
> If ".built" is not needed, please remove the search from Direcotry.mk too.
Yes, I did the same check. I have already modified my local commit to include it but did not want to push to step on your review.
>
> BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
Thanks for testing, yes it fails. It seems this is the LOADABLE case. I will look into how to fix this.
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487411120
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
I understand this is the only call to the archive being created (it is either the previous recursive call into subdirectories or this one, according to the `if`). So in this case we need to create the library from scratch. But I'm not really familiar with this SYMTABOBJ rule.
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#issuecomment-691695941
Sorry, just remeber that we can remove .built and .lock from .gitignore? @v01d
----------------------------------------------------------------
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 a change in pull request #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487385182
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
----------------------------------------------------------------
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 a change in pull request #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487385182
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build in a stable situation. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Directories that are going to be cleaned are filtered-out using this marker ".built". Removing it will keep object files (and other autogenerated files) after a clean/distclean.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Hmm.. is ".depend" covering everything?
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Is this to remove old members that are not going to get replaced with the 'r' of `ar crs`?
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built". I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build in a stable situation. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Directories that are going to be cleaned are filtered-out using this marker ".built". Removing it will keep object files (and other autogenerated files) after a clean/distclean.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Hmm.. is ".depend" covering everything?
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Is this to remove old members that are not going to get replaced with the 'r' of `ar crs`?
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built". I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build in a stable situation. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Directories that are going to be cleaned are filtered-out using this marker ".built". Removing it will keep object files (and other autogenerated files) after a clean/distclean.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Hmm.. is ".depend" covering everything?
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Is this to remove old members that are not going to get replaced with the 'r' of `ar crs`?
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built". I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build in a stable situation. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Directories that are going to be cleaned are filtered-out using this marker ".built". Removing it will keep object files (and other autogenerated files) after a clean/distclean.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Hmm.. is ".depend" covering everything?
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Is this to remove old members that are not going to get replaced with the 'r' of `ar crs`?
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built". I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build in a stable situation. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Directories that are going to be cleaned are filtered-out using this marker ".built". Removing it will keep object files (and other autogenerated files) after a clean/distclean.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Hmm.. is ".depend" covering everything?
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Is this to remove old members that are not going to get replaced with the 'r' of `ar crs`?
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built". I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build in a stable situation. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Directories that are going to be cleaned are filtered-out using this marker ".built". Removing it will keep object files (and other autogenerated files) after a clean/distclean.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Hmm.. is ".depend" covering everything?
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Is this to remove old members that are not going to get replaced with the 'r' of `ar crs`?
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built". I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build in a stable situation. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Directories that are going to be cleaned are filtered-out using this marker ".built". Removing it will keep object files (and other autogenerated files) after a clean/distclean.
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Hmm.. is ".depend" covering everything?
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Is this to remove old members that are not going to get replaced with the 'r' of `ar crs`?
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built". I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
If ".built" is not needed, please remove the search from Direcotry.mk too.
BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487401602
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
Should we use ARCHIVE_ADD here?
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#issuecomment-691696329
> Sorry, just remeber that we can remove .built and .lock from .gitignore? @v01d
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-apps] v01d commented on a change in pull request #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487411033
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Yes
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
I understand this is the only call to the archive being created (it is either the previous recursive call into subdirectories or this one, according to the `if`). So in this case we need to create the library from scratch. But I'm not really familiar with this SYMTABOBJ rule.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I will take a look at the clean.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Yes, it appears .depend covers it, don't really understand the purpose of the extra search for .built.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
> I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
> If ".built" is not needed, please remove the search from Direcotry.mk too.
Yes, I did the same check. I have already modified my local commit to include it but did not want to push to step on your review.
>
> BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
Thanks for testing, yes it fails. It seems this is the LOADABLE case. I will look into how to fix this.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I managed to solve this, I force pushed the change.
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
You were right, this needed ARCHIVE_ADD and remove the library earlier. I just pushed a change which consideres both the CONFIG_BUILD_LOADABLE and !CONFIG_BUILD_LOADABLE case.
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Yes
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
I understand this is the only call to the archive being created (it is either the previous recursive call into subdirectories or this one, according to the `if`). So in this case we need to create the library from scratch. But I'm not really familiar with this SYMTABOBJ rule.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I will take a look at the clean.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Yes, it appears .depend covers it, don't really understand the purpose of the extra search for .built.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
> I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
> If ".built" is not needed, please remove the search from Direcotry.mk too.
Yes, I did the same check. I have already modified my local commit to include it but did not want to push to step on your review.
>
> BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
Thanks for testing, yes it fails. It seems this is the LOADABLE case. I will look into how to fix this.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I managed to solve this, I force pushed the change.
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
You were right, this needed ARCHIVE_ADD and remove the library earlier. I just pushed a change which consideres both the CONFIG_BUILD_LOADABLE and !CONFIG_BUILD_LOADABLE case.
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Yes
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
I understand this is the only call to the archive being created (it is either the previous recursive call into subdirectories or this one, according to the `if`). So in this case we need to create the library from scratch. But I'm not really familiar with this SYMTABOBJ rule.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I will take a look at the clean.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Yes, it appears .depend covers it, don't really understand the purpose of the extra search for .built.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
> I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
> If ".built" is not needed, please remove the search from Direcotry.mk too.
Yes, I did the same check. I have already modified my local commit to include it but did not want to push to step on your review.
>
> BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
Thanks for testing, yes it fails. It seems this is the LOADABLE case. I will look into how to fix this.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I managed to solve this, I force pushed the change.
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
You were right, this needed ARCHIVE_ADD and remove the library earlier. I just pushed a change which consideres both the CONFIG_BUILD_LOADABLE and !CONFIG_BUILD_LOADABLE case.
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Yes
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
I understand this is the only call to the archive being created (it is either the previous recursive call into subdirectories or this one, according to the `if`). So in this case we need to create the library from scratch. But I'm not really familiar with this SYMTABOBJ rule.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I will take a look at the clean.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Yes, it appears .depend covers it, don't really understand the purpose of the extra search for .built.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
> I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
> If ".built" is not needed, please remove the search from Direcotry.mk too.
Yes, I did the same check. I have already modified my local commit to include it but did not want to push to step on your review.
>
> BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
Thanks for testing, yes it fails. It seems this is the LOADABLE case. I will look into how to fix this.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I managed to solve this, I force pushed the change.
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
You were right, this needed ARCHIVE_ADD and remove the library earlier. I just pushed a change which consideres both the CONFIG_BUILD_LOADABLE and !CONFIG_BUILD_LOADABLE case.
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Yes
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
I understand this is the only call to the archive being created (it is either the previous recursive call into subdirectories or this one, according to the `if`). So in this case we need to create the library from scratch. But I'm not really familiar with this SYMTABOBJ rule.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I will take a look at the clean.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Yes, it appears .depend covers it, don't really understand the purpose of the extra search for .built.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
> I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
> If ".built" is not needed, please remove the search from Direcotry.mk too.
Yes, I did the same check. I have already modified my local commit to include it but did not want to push to step on your review.
>
> BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
Thanks for testing, yes it fails. It seems this is the LOADABLE case. I will look into how to fix this.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I managed to solve this, I force pushed the change.
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
You were right, this needed ARCHIVE_ADD and remove the library earlier. I just pushed a change which consideres both the CONFIG_BUILD_LOADABLE and !CONFIG_BUILD_LOADABLE case.
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Yes
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
I understand this is the only call to the archive being created (it is either the previous recursive call into subdirectories or this one, according to the `if`). So in this case we need to create the library from scratch. But I'm not really familiar with this SYMTABOBJ rule.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I will take a look at the clean.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Yes, it appears .depend covers it, don't really understand the purpose of the extra search for .built.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
> I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
> If ".built" is not needed, please remove the search from Direcotry.mk too.
Yes, I did the same check. I have already modified my local commit to include it but did not want to push to step on your review.
>
> BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
Thanks for testing, yes it fails. It seems this is the LOADABLE case. I will look into how to fix this.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I managed to solve this, I force pushed the change.
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
You were right, this needed ARCHIVE_ADD and remove the library earlier. I just pushed a change which consideres both the CONFIG_BUILD_LOADABLE and !CONFIG_BUILD_LOADABLE case.
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Yes
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
I understand this is the only call to the archive being created (it is either the previous recursive call into subdirectories or this one, according to the `if`). So in this case we need to create the library from scratch. But I'm not really familiar with this SYMTABOBJ rule.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I will take a look at the clean.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Yes, it appears .depend covers it, don't really understand the purpose of the extra search for .built.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
> I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
> If ".built" is not needed, please remove the search from Direcotry.mk too.
Yes, I did the same check. I have already modified my local commit to include it but did not want to push to step on your review.
>
> BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
Thanks for testing, yes it fails. It seems this is the LOADABLE case. I will look into how to fix this.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I managed to solve this, I force pushed the change.
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
You were right, this needed ARCHIVE_ADD and remove the library earlier. I just pushed a change which consideres both the CONFIG_BUILD_LOADABLE and !CONFIG_BUILD_LOADABLE case.
##########
File path: Makefile
##########
@@ -87,6 +87,10 @@ else
ifeq ($(CONFIG_BUILD_LOADABLE),)
$(BIN): $(foreach SDIR, $(CONFIGURED_APPS), $(SDIR)_all)
+ $(RM) $(BIN)
Review comment:
Yes
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
I understand this is the only call to the archive being created (it is either the previous recursive call into subdirectories or this one, according to the `if`). So in this case we need to create the library from scratch. But I'm not really familiar with this SYMTABOBJ rule.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I will take a look at the clean.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
Yes, it appears .depend covers it, don't really understand the purpose of the extra search for .built.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
> I tried again to see if there is any leftover if we omit the search for ".built", but things seem clean. I'm not sure if there is a place where only .built is generated. We need to let the checks of this PR run to know (after we merge the dependency).
> If ".built" is not needed, please remove the search from Direcotry.mk too.
Yes, I did the same check. I have already modified my local commit to include it but did not want to push to step on your review.
>
> BTW, can you build stm32f4discovery:wifi? I pulled your changes (both os&apps) I can build other cofings like esp32-core:nsh but the stm32f4discovery:wifi is giving me the libapp.a file not found error.
Thanks for testing, yes it fails. It seems this is the LOADABLE case. I will look into how to fix this.
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I managed to solve this, I force pushed the change.
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
You were right, this needed ARCHIVE_ADD and remove the library earlier. I just pushed a change which consideres both the CONFIG_BUILD_LOADABLE and !CONFIG_BUILD_LOADABLE case.
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487430307
##########
File path: Makefile
##########
@@ -100,9 +104,9 @@ $(SYMTABOBJ): %$(OBJEXT): %.c
$(BIN): $(SYMTABOBJ)
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE, "${shell cygpath -w $(BIN)}", $^)
Review comment:
You were right, this needed ARCHIVE_ADD and remove the library earlier. I just pushed a change which consideres both the CONFIG_BUILD_LOADABLE and !CONFIG_BUILD_LOADABLE case.
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487549775
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Please don't forget to remove the flock from CI script and the document.
----------------------------------------------------------------
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 a change in pull request #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487385182
##########
File path: Make.defs
##########
@@ -108,10 +108,6 @@ define REGISTER
$(Q) touch "$(BUILTIN_REGISTRY)$(DELIM).updated"
endef
-define ARLOCK
- $(Q) flock $1.lock $(call ARCHIVE, $1, $(2))
Review comment:
Hmmmm flock was necessary to get the nightly build to stabilise. @xiaoxiang781216 could you please comment here.
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#issuecomment-691731421
> LGTM, @Ouss4 please merge it if you are fine too.
@xiaoxiang781216 did you had the chance to take a look at https://github.com/apache/incubator-nuttx/pull/1765? If all is good I'd like that we merge that one first and restart the checks here.
I just want to make sure we won't have any leftovers. Some configs are tricky.
----------------------------------------------------------------
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 #380: Fix: ensure archive files do not carry object files from prior builds
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #380:
URL: https://github.com/apache/incubator-nuttx-apps/pull/380#discussion_r487411134
##########
File path: Application.mk
##########
@@ -131,13 +131,12 @@ $(CXXOBJS): %$(SUFFIX)$(OBJEXT): %$(CXXEXT)
$(if $(and $(CONFIG_BUILD_LOADABLE),$(CXXELFFLAGS)), \
$(call ELFCOMPILEXX, $<, $@), $(call COMPILEXX, $<, $@))
-.built: $(OBJS)
+link:
ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
- $(call ARLOCK, "${shell cygpath -w $(BIN)}", $^)
+ $(call ARCHIVE_ADD, "${shell cygpath -w $(BIN)}", $(OBJS))
else
- $(call ARLOCK, $(BIN), $^)
+ $(call ARCHIVE_ADD, $(BIN), $(OBJS))
endif
- $(Q) touch $@
Review comment:
I will take a look at the clean.
----------------------------------------------------------------
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