You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/05/30 19:58:58 UTC
[GitHub] [incubator-nuttx] patacongo opened a new pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
patacongo opened a new pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152
## Summary
There were several cases where there were attempts to use the tools/incdir binary before it was used. This resolves that issue (adding a few seconds to build time).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636391997
> You are exactly right. I think we could fix this by doing 'make tools/incdir' before doing 'make olddefconfig' in sethost.sh (and configure.c)
Done... Now this PR should build with no more failures to find the incdir binary. The change has only been made to sethost.sh. I will make the corresponding change to configure.sh is this PR is approved.
This is not a pretty change. If anyone has better ideas, let me know.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636386137
Backing out the changes to Config.mk restores good build performance:
real 0m35.451s
user 0m24.371s
sys 1m26.637s
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636391997
> You are exactly right. I think we could fix this by doing 'make tools/incdir' before doing 'make olddefconfig' in sethost.sh (and configure.c)
Done... Now this PR should build with no more failures to find the incdir binary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#discussion_r432883682
##########
File path: tools/Makefile.unix
##########
@@ -308,7 +308,7 @@ ifneq ($(CONFIG_ARCH_CHIP),)
$(Q) touch $@
endif
-dirlinks: include/arch include/arch/board include/arch/chip $(ARCH_SRC)/board $(ARCH_SRC)/chip drivers/platform
+dirlinks: tools/incdir$(HOSTEXEEXT) include/arch include/arch/board include/arch/chip $(ARCH_SRC)/board $(ARCH_SRC)/chip drivers/platform
Review comment:
Should `incdir` be removed from pass1/2 targets now?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636464350
> but we still need do some hack before invoke Makefile.host since Makefile.host also include Make.defs too
That hack is already in `${shell $(MAKE) -C tools -f Makefile.host incdir INCDIR="$(TOPDIR)/tools/incdir.sh"}` `INCDIR` is given an initial value of `tools/incdir.sh` so that Makefile.host doesn't complain.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636391997
> You are exactly right. I think we could fix this by doing 'make tools/incdir' before doing 'make olddefconfig' in sethost.sh (and configure.c)
Done... Now this PR should build with no more failures to find the incdir binary. The change has only been made to sethost.sh. I will make the corresponding change to configure.sh is this PR is approved.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636427076
@patacongo Yes, it is better to put the logic into Makefile than refresh.sh, but we still need do some hack before invoke Makefile.host since Makefile.host also include Make.defs too. My question is that how much benefit we can get from the immediate varaible?
https://github.com/apache/incubator-nuttx/pull/1150
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636410048
>
>
> PS.: In the checks there are some `/github/workspace/sources/nuttx/tools/incdir.exe: not found` these are coming from tools/refresh.sh
I think these would be fixed by the propose logic added to the top-level Makefile above. These are probably coming from 'make olddefconfig' and 'make savedefconfig' with no tools/incdir binary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636390421
> ```makefile
> CINCPATH := ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
> CXXINCPATH := ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
> ```
You are exactly right. I think we could fix this by doing 'make tools/incdir' before doing 'make olddefconfig' in sethost.sh (and configure.c)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636386673
> If we accept the changes to Makefile.win and Makefile.unix but ignor the bad changes to Config.mk, then all of the errors about failing to find the incdir binary disappear except four that occur during configuration.
I have backed out the changes to Config.mk. That means that there are still 4 reported errors at configuration time. I am not sure yet how to fix these.
The remaining changes to Makefile.unix and Makefile.win are still good, but insufficient because you will still see 4 errors about failing to find the tools/incdir binary.
For the record, this is the change to Config.mk that was backed out:
diff --git a/tools/Config.mk b/tools/Config.mk
index 1e163ce03c..2ba0b3779f 100644
--- a/tools/Config.mk
+++ b/tools/Config.mk
@@ -174,15 +174,18 @@ endif
#
# CONFIG_WINDOWS_NATIVE - Defined for a Windows native build
+INCDIRSH := "$(TOPDIR)/tools/incdir.sh"
+INCDIRBIN := "$(TOPDIR)/tools/incdir$(HOSTEXEEXT)"
+
ifeq ($(CONFIG_WINDOWS_NATIVE),y)
DEFINE ?= "$(TOPDIR)\tools\define.bat"
INCDIR ?= "$(TOPDIR)\tools\incdir.bat"
else ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
DEFINE ?= "$(TOPDIR)/tools/define.sh" -w
- INCDIR ?= "$(TOPDIR)/tools/incdir$(HOSTEXEEXT)" -w
+ INCDIR ?= $(if $(wildcard $(INCDIRBIN)),$(INCDIRBIN) -w,$(INCDIRSH) -w)
else
DEFINE ?= "$(TOPDIR)/tools/define.sh"
- INCDIR ?= "$(TOPDIR)/tools/incdir$(HOSTEXEEXT)"
+ INCDIR ?= $(if $(wildcard $(INCDIRBIN)),$(INCDIRBIN),$(INCDIRSH))
endif
# PREPROCESS - Default macro to run the C pre-processor
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636407106
> I think we could fix this by doing 'make tools/incdir' before doing 'make olddefconfig' in sethost.sh (and configure.c)
A better place would be from the top-level Makefile. It does not include Make.defs and could create tools/incdir prior to include Makefile.unix. That would be cleaner and more self-contained to the build system.
This works but is not pretty:
$ git diff -- Makefile
diff --git a/Makefile b/Makefile
index ea1866ad4f..3b3a26f5a2 100644
--- a/Makefile
+++ b/Makefile
@@ -47,6 +47,8 @@ ifeq ($(wildcard .config),)
@echo " tools/configure.sh <target>"
else
include .config
+TOPDIR := ${shell echo $(CURDIR) | sed -e 's/ /\\ /g'}
+DUMMY := ${shell $(MAKE) -C tools -f Makefile.host incdir INCDIR="$(TOPDIR)/tools/incdir.sh"}
ifeq ($(CONFIG_WINDOWS_NATIVE),y)
include tools/Makefile.win
else
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo removed a comment on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo removed a comment on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636386673
> If we accept the changes to Makefile.win and Makefile.unix but ignor the bad changes to Config.mk, then all of the errors about failing to find the incdir binary disappear except four that occur during configuration.
I have backed out the changes to Config.mk. That means that there are still 4 reported errors at configuration time. I am not sure yet how to fix these.
The remaining changes to Makefile.unix and Makefile.win are still good, but insufficient because you will still see 4 errors about failing to find the tools/incdir binary.
For the record, this is the change to Config.mk that was backed out:
diff --git a/tools/Config.mk b/tools/Config.mk
index 1e163ce03c..2ba0b3779f 100644
--- a/tools/Config.mk
+++ b/tools/Config.mk
@@ -174,15 +174,18 @@ endif
#
# CONFIG_WINDOWS_NATIVE - Defined for a Windows native build
+INCDIRSH := "$(TOPDIR)/tools/incdir.sh"
+INCDIRBIN := "$(TOPDIR)/tools/incdir$(HOSTEXEEXT)"
+
ifeq ($(CONFIG_WINDOWS_NATIVE),y)
DEFINE ?= "$(TOPDIR)\tools\define.bat"
INCDIR ?= "$(TOPDIR)\tools\incdir.bat"
else ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
DEFINE ?= "$(TOPDIR)/tools/define.sh" -w
- INCDIR ?= "$(TOPDIR)/tools/incdir$(HOSTEXEEXT)" -w
+ INCDIR ?= $(if $(wildcard $(INCDIRBIN)),$(INCDIRBIN) -w,$(INCDIRSH) -w)
else
DEFINE ?= "$(TOPDIR)/tools/define.sh"
- INCDIR ?= "$(TOPDIR)/tools/incdir$(HOSTEXEEXT)"
+ INCDIR ?= $(if $(wildcard $(INCDIRBIN)),$(INCDIRBIN),$(INCDIRSH))
endif
# PREPROCESS - Default macro to run the C pre-processor
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636408299
PS.: In the checks there are some `/github/workspace/sources/nuttx/tools/incdir.exe: not found` these are coming from tools/refresh.sh
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636384570
> I am thinking that the cause of the performance degradation is that INCDIR is defined only once when Config.mk is first included. After the true tools/incdir binary is created, the INCDIR definition is not updated and continues to use incdir.sh, ruining perfromance.
If we accept the changes to Makefile.win and Makefile.unix but ignor the bad changes to Config.mk, then all of the errors about failing to find the indir binary disapper except four that occur during configuration.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 merged pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
Ouss4 merged pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#discussion_r432886561
##########
File path: tools/Makefile.unix
##########
@@ -308,7 +308,7 @@ ifneq ($(CONFIG_ARCH_CHIP),)
$(Q) touch $@
endif
-dirlinks: include/arch include/arch/board include/arch/chip $(ARCH_SRC)/board $(ARCH_SRC)/chip drivers/platform
+dirlinks: tools/incdir$(HOSTEXEEXT) include/arch include/arch/board include/arch/chip $(ARCH_SRC)/board $(ARCH_SRC)/chip drivers/platform
Review comment:
It just won't be needed because it would've already been built during dirlinks (which is also a part of context)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636388736
Here are the remaining four errors reported. This is only during configuration:
$ make olddefconfig V=1
/bin/sh: [HOME]/nuttx/tools/incdir.exe: No such file or directory
/bin/sh: [HOME]/nuttx/tools/incdir.exe: No such file or directory
make -C tools -f Makefile.host TOPDIR="[HOME]/nuttx" incdir.exe
make[1]: Entering directory '[HOME]/nuttx/tools'
/bin/sh: [HOME]/nuttx/tools/incdir.exe: No such file or directory
/bin/sh: [HOME]/nuttx/tools/incdir.exe: No such file or directory
gcc -Wall -Wstrict-prototypes -Wshadow -Wundef -g -pipe -DHAVE_STRTOK_C=1 -DHOST_CYGWIN=1 -o incdir.exe incdir.c
Where [HOME]/nuttx is the location on my machine of the incubator_nuttx repository.
Any body got any ideas where these are coming from?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636427076
@patacongo Yes, it is better to put the logic into Makefile than refresh.sh, but we still need do some hack before invoke Makefile.host since Makefile.host also include Make.defs too. My question is that how much benefit we can get from the immediate varaible?
https://github.com/apache/incubator-nuttx/pull/1150
If the value isn't too huge, we can keep the delayed variable as before to simplify the logic.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636407106
> I think we could fix this by doing 'make tools/incdir' before doing 'make olddefconfig' in sethost.sh (and configure.c)
A better place would be from the top-level Makefile. It does not include Make.defs and could create tools/incdir prior to include Makefile.unix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636389166
> Does anyone have any idea where these are coming from?
We know they are coming from the Make.defs file, but we don't know how to get rid of them (i.e. build incdir before getting here)
```Make
CINCPATH := ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include}
CXXINCPATH := ${shell $(INCDIR) -s "$(CC)" $(TOPDIR)$(DELIM)include$(DELIM)cxx}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636407106
> I think we could fix this by doing 'make tools/incdir' before doing 'make olddefconfig' in sethost.sh (and configure.c)
A better place would be from the top-level Makefile. It does not include Make.defs and could create tools/incdir prior to include Makefile.unix. That would be cleaner and more self-contained to the build system. And eliminates the requirement that the tools/incdir binary exist prior to the first make.
This works but is not pretty:
$ git diff -- Makefile
diff --git a/Makefile b/Makefile
index ea1866ad4f..3b3a26f5a2 100644
--- a/Makefile
+++ b/Makefile
@@ -47,6 +47,8 @@ ifeq ($(wildcard .config),)
@echo " tools/configure.sh <target>"
else
include .config
+TOPDIR := ${shell echo $(CURDIR) | sed -e 's/ /\\ /g'}
+DUMMY := ${shell $(MAKE) -C tools -f Makefile.host incdir INCDIR="$(TOPDIR)/tools/incdir.sh"}
ifeq ($(CONFIG_WINDOWS_NATIVE),y)
include tools/Makefile.win
else
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636386673
> If we accept the changes to Makefile.win and Makefile.unix but ignor the bad changes to Config.mk, then all of the errors about failing to find the incdir binary disappear except four that occur during configuration.
I have backed out the changes to Config.mk. That means that there are still 4 reported errors at configuration time. I am not sure yet how to do these.
The remaining changes to Makefile.unix and Makefile.win are still good, but insufficient because you will still see 4 errors about failing to find the tools/incdir binary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636386673
> If we accept the changes to Makefile.win and Makefile.unix but ignor the bad changes to Config.mk, then all of the errors about failing to find the incdir binary disappear except four that occur during configuration.
I have backed out the changes to Config.mk. That means that there are still 4 reported errors at configuration time. I am not sure yet how to do these.
The remaining changes to Makefile.unix and Makefile.win are still good, but insufficient because you will still see 4 errors about failing to find the tools/incdir binary.
For the record, this is the change to Config.mk that was backed out:
diff --git a/tools/Config.mk b/tools/Config.mk
index 1e163ce03c..2ba0b3779f 100644
--- a/tools/Config.mk
+++ b/tools/Config.mk
@@ -174,15 +174,18 @@ endif
#
# CONFIG_WINDOWS_NATIVE - Defined for a Windows native build
+INCDIRSH := "$(TOPDIR)/tools/incdir.sh"
+INCDIRBIN := "$(TOPDIR)/tools/incdir$(HOSTEXEEXT)"
+
ifeq ($(CONFIG_WINDOWS_NATIVE),y)
DEFINE ?= "$(TOPDIR)\tools\define.bat"
INCDIR ?= "$(TOPDIR)\tools\incdir.bat"
else ifeq ($(CONFIG_CYGWIN_WINTOOL),y)
DEFINE ?= "$(TOPDIR)/tools/define.sh" -w
- INCDIR ?= "$(TOPDIR)/tools/incdir$(HOSTEXEEXT)" -w
+ INCDIR ?= $(if $(wildcard $(INCDIRBIN)),$(INCDIRBIN) -w,$(INCDIRSH) -w)
else
DEFINE ?= "$(TOPDIR)/tools/define.sh"
- INCDIR ?= "$(TOPDIR)/tools/incdir$(HOSTEXEEXT)"
+ INCDIR ?= $(if $(wildcard $(INCDIRBIN)),$(INCDIRBIN),$(INCDIRSH))
endif
# PREPROCESS - Default macro to run the C pre-processor
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#discussion_r432884122
##########
File path: tools/Makefile.unix
##########
@@ -308,7 +308,7 @@ ifneq ($(CONFIG_ARCH_CHIP),)
$(Q) touch $@
endif
-dirlinks: include/arch include/arch/board include/arch/chip $(ARCH_SRC)/board $(ARCH_SRC)/chip drivers/platform
+dirlinks: tools/incdir$(HOSTEXEEXT) include/arch include/arch/board include/arch/chip $(ARCH_SRC)/board $(ARCH_SRC)/chip drivers/platform
Review comment:
why? It still needs to be built. The change to Config.mk doesn't build it. It just prevents using it if it is not built.
I don't understand what would happen if you remove the dependency from pass1 and pass1. I think 'make pass1 pass2' would then fail, correct? Or it would just be very slow because the incdir binary was never built.
I believe the build would fail without these. But you should give it a try.
'make pass1 pass2' is actually recommended in some README.txt files.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636401262
@xiaoxiang781216 One very negative aspect of this whole change to improve performance by using a compiled C tools/incdir binary is that the tools/incdir binary must exist prior to the first make.
The main reason for that is because the Make.defs files all need to add use $INCDIR to create CFLAGS but INCDIR refers to the the tools/incdir binary which has not been built yet.
The solution for now is to add logic into sethost.sh so that the tools/incdir binary is built before the first 'make olddefconfigs'. This is not a clean solution and if you have some better ideas, I would love to hear them. You usually have some pretty good ideas!
Abdelatif has a number of clean-ups that he would also like to do in Makefile.win/unix but that is based on the notion that the tools/incdir binary exists prior to make. I asked him to hold off on those changes until we get some feedback from you.
The changes in the PR are sufficient to fix all of the silent errors that we were seeing the PR check logs. They can be merged as is if you like. Or we can incorporate any new, good ideas they you may have.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636407106
> I think we could fix this by doing 'make tools/incdir' before doing 'make olddefconfig' in sethost.sh (and configure.c)
A better place would be from the top-level Makefile. It does not include Make.defs and could create tools/incdir prior to include Makefile.unix.
This works but is not pretty:
$ git diff -- Makefile
diff --git a/Makefile b/Makefile
index ea1866ad4f..3b3a26f5a2 100644
--- a/Makefile
+++ b/Makefile
@@ -47,6 +47,8 @@ ifeq ($(wildcard .config),)
@echo " tools/configure.sh <target>"
else
include .config
+TOPDIR := ${shell echo $(CURDIR) | sed -e 's/ /\\ /g'}
+DUMMY := ${shell $(MAKE) -C tools -f Makefile.host incdir INCDIR="$(TOPDIR)/tools/incdir.sh"}
ifeq ($(CONFIG_WINDOWS_NATIVE),y)
include tools/Makefile.win
else
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636388736
Here are the remaining four errors reported. This is only during configuration:
$ make olddefconfig V=1
/bin/sh: [HOME]/nuttx/tools/incdir.exe: No such file or directory
/bin/sh: [HOME]/nuttx/tools/incdir.exe: No such file or directory
make -C tools -f Makefile.host TOPDIR="[HOME]/nuttx" incdir.exe
make[1]: Entering directory '[HOME]/nuttx/tools'
/bin/sh: [HOME]/nuttx/tools/incdir.exe: No such file or directory
/bin/sh: [HOME]/nuttx/tools/incdir.exe: No such file or directory
gcc -Wall -Wstrict-prototypes -Wshadow -Wundef -g -pipe -DHAVE_STRTOK_C=1 -DHOST_CYGWIN=1 -o incdir.exe incdir.c
Where [HOME]/nuttx is the location on my machine of the incubator_nuttx repository.
Does anyone have any idea where these are coming from?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #1152: tools/Makefiles: Fix use of unbuilt incdir binary
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #1152:
URL: https://github.com/apache/incubator-nuttx/pull/1152#issuecomment-636475978
@xiaoxiang781216 Giving the time difference I don't think you'd be available now, so I'd like to merge this PR because:
1. It removes the error messages, we don't want them to stick around for too long, and
2. We'd like to see how this change will do during the nightly build.
Please continue reviewing this and we will address any concern you might have.
----------------------------------------------------------------
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