Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Makefile: move INCDIROPT to common place #625

Merged
merged 1 commit into from
Mar 26, 2020
Merged

Makefile: move INCDIROPT to common place #625

merged 1 commit into from
Mar 26, 2020

Conversation

xiaoxiang781216
Copy link
Contributor

Signed-off-by: Xiang Xiao [email protected]

@patacongo
Copy link
Contributor

patacongo commented Mar 26, 2020 •
edited
Loading

This change effects systems using Cygwin under windows with Windos native toolchain. In that case WINTOOLS will be defined and the -w option must be used. Has that been verified? This change should not be merged if that test case has not been verified.

This is the build configuration that I use Windows Cygwin plus Windows native toolchan and this looks to me like it would break my build.

Mmm.. Isee that the -w was added in Config.mk. Perhaps there is no issue. We need to have a test case for this configuration! Merging the change is a very big risk with no testing.

@patacongo patacongo merged commit b9ace36 into apache:master Mar 26, 2020
@xiaoxiang781216
Copy link
Contributor Author

This change effects systems using Cygwin under windows with Windos native toolchain. In that case WINTOOLS will be defined and the -2 option must be used. Has that been verified. This change should not be merged if that test case has not been verified.

Yes, we need improve github/apache to verify the patch for windows cygwin/native/msys2.

It appears to me that the -w was only removed, breaking all such builds. This is the build configuration that I use Windows Cygwin plus Windows native toolchan and this looks to me like it would break my build.

-w doesn't lose, I set the -w in tools/Config.mk

https://github.com/apache/incubator-nuttx/pull/625/files#diff-043fcc977f97645d33de793ab09bd262R99

I think it should not be merged.

@patacongo
Copy link
Contributor

Should there be a companion change to incubator-nuttx-apps?

@patacongo
Copy link
Contributor

patacongo commented Mar 26, 2020 •
edited
Loading

No, it does not appear to work. My system no longer builds. Looking into it. Never mind, seems to have been a bad initial setup. I do see this error during dependency generation:

make[1]: Entering directory '/cygdrive/d/Spuda/Documents/projects/nuttx/master/incubator_nuttx/binfmt'
.\binfmt_execmodule.c:54:10: fatal error: sched/sched.h: No such file or directory
 #include "sched/sched.h"
          ^~~~~~~~~~~~~~~
compilation terminated.
ERROR: arm-none-eabi-gcc failed: 1
       command: arm-none-eabi-gcc -M -fno-builtin -funwind-tables -Wall -Wstrict-prototypes -Wshadow -Wundef -Os -fno-strict-aliasing -fomit-frame-pointer -fno-strength-reduce -mcpu=cortex-m4 -mthumb -mfloat-abi=soft -I. -isystem D:\\Spuda\\Documents\\projects\\nuttx\\master\\incubator_nuttx\\include -D__KERNEL__ -pipe -I /cygdrive/d/Spuda/Documents/projects/nuttx/master/incubator_nuttx/sched .\\binfmt_execmodule.c
make[1]: *** [Makefile:93: .depend] Error 1

Looks like the code is breaking modularity rules.

@xiaoxiang781216
Copy link
Contributor Author

Should there be a companion change to incubator-nuttx-apps?

Yes, here: apache/nuttx-apps#141

@patacongo
Copy link
Contributor

patacongo commented Mar 26, 2020 •
edited
Loading

The same error occurs during compilation:

CC:  binfmt_execmodule.c
binfmt_execmodule.c:54:10: fatal error: sched/sched.h: No such file or directory
 #include "sched/sched.h"
          ^~~~~~~~~~~~~~~
compilation terminated.

So I am unable to verify the changes. But quite a bit did compile.

@xiaoxiang781216
Copy link
Contributor Author

Should there be a companion change to incubator-nuttx-apps?

Yes, here: apache/incubator-nuttx-apps#141

No, it does not appear to work. My system no longer builds. Looking into it.

It's time to create wndows VM to very all PR for cygwin/native/msys.
@liuguo09 could you work with @patacongo to setup Windows CI environment?

@patacongo
Copy link
Contributor

patacongo commented Mar 26, 2020 •
edited
Loading

Whatever binfmt_execmodule.c needs it should not include sched/sched.h directly. the necessary things need to be moved into include/nuttx and binfmt must not include "sched/sched.h".

It looks like I did this in commit d798dd3 . that should be fixed. Wow... that was back in 2014.

@patacongo
Copy link
Contributor

It looks like I did this in commit d798dd3 . that should be fixed. Wow... that was back in 2014.

So that change is not the direct, root cause of the problem. Rather some change in the include path has broken that build. Let me check. yes. I think this change broke the build.

make -C binfmt TOPDIR="/cygdrive/d/Spuda/Documents/projects/nuttx/master/incubator_nuttx" libbinfmt.a KERNEL=y EXTRADEFINES=-D__KERNEL__
make[1]: Entering directory '/cygdrive/d/Spuda/Documents/projects/nuttx/master/incubator_nuttx/binfmt'
CC:  binfmt_execmodule.c
arm-none-eabi-gcc -c -fno-builtin -funwind-tables -Wall -Wstrict-prototypes -Wshadow -Wundef -Os -fno-strict-aliasing -fomit-frame-pointer -fno-strength-reduce -mcpu=cortex-m4 -mthumb -mfloat-abi=soft -I. -isystem "D:\Spuda\Documents\projects\nuttx\master\incubator_nuttx\include"  -D__KERNEL__ -pipe -I "/cygdrive/d/Spuda/Documents/projects/nuttx/master/incubator_nuttx/sched"   binfmt_execmodule.c -o  binfmt_execmodule.o
binfmt_execmodule.c:54:10: fatal error: sched/sched.h: No such file or directory
 #include "sched/sched.h"
          ^~~~~~~~~~~~~~~
compilation terminated.

This is the argumentthat caused the problem:

-I "/cygdrive/d/Spuda/Documents/projects/nuttx/master/incubator_nuttx/sched"

That is a Cygwin path, not a Windows path. The tool requires a Windows path (i.e., with the -w option).

@patacongo
Copy link
Contributor

patacongo commented Mar 26, 2020 •
edited
Loading

I think that the problem is in binfmt/Makefile.

 40 CFLAGS += ${shell $(INCDIR) "$(CC)" "$(TOPDIR)$(DELIM)sched"}

It includes $(TOPDIR)/Make.defs, but apparently does not get the -w option?

Any, the build is broken for all Cygwin uses which is the largest part of the user base.

@patacongo
Copy link
Contributor

Should I revert the change? Or can you recommend a fix?

@patacongo
Copy link
Contributor

[I know the problem. WINTOOL is not available to Config.mk. It is included by Make.defs BEFORE WINTOOL is defined. This must be reverted.

patacongo pushed a commit to patacongo/nuttx that referenced this pull request Mar 26, 2020
@gregory-nutt
Revert "Makefile: move INCDIROPT to common place (apache#625)"
304cecf 
This reverts commit b9ace36.

This change was added by PR 625 but has a serious logic flaw.  It removes all occurrences of INCDIROPT and replaces it with a definition in tools/Config.mk:

    else ifeq ($(WINTOOL),y)
      DEFINE = "$(TOPDIR)/tools/define.sh"
      INCDIR = "$(TOPDIR)/tools/incdir.sh" -w

This logic flaw is the Config.mk is included in all Make.defs files BEFORE WINTOOL is defined.  As a result, the definition is wrong in many places when building under Cygwin with a Windows native toolchain.
@patacongo patacongo mentioned this pull request Mar 26, 2020
Merged
btashton pushed a commit that referenced this pull request Mar 26, 2020
@gregory-nutt @btashton
Revert "Makefile: move INCDIROPT to common place (#625)"
5c0e8e8 
This reverts commit b9ace36.

This change was added by PR 625 but has a serious logic flaw.  It removes all occurrences of INCDIROPT and replaces it with a definition in tools/Config.mk:

    else ifeq ($(WINTOOL),y)
      DEFINE = "$(TOPDIR)/tools/define.sh"
      INCDIR = "$(TOPDIR)/tools/incdir.sh" -w

This logic flaw is the Config.mk is included in all Make.defs files BEFORE WINTOOL is defined.  As a result, the definition is wrong in many places when building under Cygwin with a Windows native toolchain.
@patacongo patacongo mentioned this pull request Mar 26, 2020
Merged
@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Mar 26, 2020 •
edited
Loading

[I know the problem. WINTOOL is not available to Config.mk. It is included by Make.defs BEFORE WINTOOL is defined. This must be reverted.

Ok, I will find a method ensure WINTOOL get define before include Config.mk

@patacongo
Copy link
Contributor

I fixed the issues with binfmt/binfmt_execmodule.c with PR #632 and I cannot find any other location with this exact failure setup, This does not fix the logical, ordering problem in general so I don't think recommends the PR.

The best fix would be to make WINTOOL into a configuration option CONFIG_BUILD_WINTOOL that is set automatically if a Windows native toolcahin is selected and if the platform is Cygwin. Thent he definition with exist prior to the first inclusion of Config.mk.

Currently WINTOOL is set in the Toolchain.defs file. It would affect several Kconfig files to make it a pre-existing configuration setting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xiaoxiang781216 @patacongo