-
Notifications
You must be signed in to change notification settings - Fork 21
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.
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
WIP: Add Cygwin to CI #39
base: master
Are you sure you want to change the base?
Conversation
@btashton can you split the Fetch-Source to another patch? so we can sync build.yaml with other project first, and then focus on Windows CI specific change. |
Yeah I needed to test a modification for this repo, now that it is tested I can do that, but it will need to wait until tomorrow, it's late here. |
I'm going to close this for now. We can always refer to it again later. |
All right since we had another failure with Windows I will bring this back, but I think we should scope it to just run the |
Using sim will not catch breakage like that of PR #1450. That error is cause due to running and Windows native toolchain (in this case the the ARM embedded toolchain) to compile code under Cygwin. I have been using sim with Cygwin and never experienced any problems. |
Ok. I thought you had said something about not being about to use the native ARM Embedded toolchain with Cygwin, maybe I am missing something. Either way I'll make this is a priority since it is a big hole. If I get this cleaned up next week (right now it has unrelated code changes) can you give me a little guidance on the toolchain. |
Semantic problem. I am not being clear. The Windows native ARM cross-compiler toolchain -- like the ARM embedded toolchain. Versus a toolchain (like buildroot) that is compiled under Cygwin. And not the Windows native x86_64 toolchain. No problem with the last two. The word native was not clear in this context. |
I think you could replicate the problem with the sim platform if you used a Windows native GCC tool chain like MinGW. The pre-built Cygwin gcc is built under Cygwin and everything will use POSIX paths. That environment is indistiguishable for the most part from Linux. But MinGW (I think) will not understand POSIX paths and will general Windows paths. I have never experimented with that so I am pushing things. |
Here is the crux of the problem under Cygwin:
The latter is handled with no problem, the former fails when the make $(dir ...) command attempts to extract the path to libgcc.a. It needs a POSIX style path. 'cypath -u' can fix that. |
bb42d72
to
2bf71c6
Compare
@patacongo @v01d Getting this closer to being in a reasonable place, but it looks like there is an issue with These folders are getting left behind:
Here is a snip of the log:
|
Great work, it will saves time in the future avoiding build breaks. I'm not really familiar with cygwin to understand the breakage. But I suspect it has something to do with the script that handle symlinks. @patacongo any ideas? |
I never noticed, but yes it is a real problem. I have several old board and and chip directories. We need to open an issue. These two lines in arch/arm/src/Makefile (and other arch Makefiles) are not working under cygwin:
Cygwin supports is own style of symbolic links and these are used EXCEPT for the case where a Windows native toolchain is used such as the popular (https://developer.arm.com/tools-and-software/open-source-software/developer-tools/gnu-toolchain/gnu-rm). Those are Window native tools and cannot follow non-standard Cygwin symbolic links so in this case, the tools/dirlinks.sh script does a full directory copy and the corresponding unlink script does a directory removal. Without knowing more, it looks like the DIRUNLINK is failing to remove the Cygwin symbolic link or the copied directory. |
The dir unlink script seems to be working fine, however (at least using a relative path):
|
Oops.. didn't mean to close. |
See Issue apache/nuttx#1880 |
@v01d @btashton I have analyzed this. It is due to changes make in the build system that were never verified on Cygwin. I have documented the problem in apache/nuttx#1880 I don't have any good solution other than reverting some changes recently made to the Makefile. |
@patacongo I have to step away for the day (new job so back to evenings, mornings and weekends) but I appreciate you digging into it and I'll do what I can to make sure we have the protections in place via CI going forward. MinGW will also need to be added at some point. I suspect that will actually be easier since the runners are already configured with the core packages. |
The good news is that your CI changes work! The correctly caught an error introduced into the build system. |
1ac2d95
to
051ef3f
Compare
I just tweaked the workflow to so we can get a full run with all of the errors. I looks like there are still a couple issues with the build we need to address, but @liuguo09 patch did fix the folder issue. |
@patacongo @xiaoxiang78121 The build is taking a long time, but I am seeing a handful of different classes of errors. What do you think about enabling the cygwin CI, but making it not fail the workflow. That way we can more easily validate the fixes, but also not fail unrelated PRs? |
Looks cpp related
make distclean is then leaving deps around
|
These all look like toolchain-related issues? Which toolchain are you using? I recommend the ARM embedded toolchain which is a windows native toolchain. All of the problems that I see could be explained if CONFIG_CYGWIN_WINTOOL were not set: The -I and -isystem arguments would be wrong in this case:
The following error often happens in that case:
That is because the windows toolchain cannot follow the cygwin paths. Can you verify that |
This also can occur if the links are Cygwin symbolic links. In that case, the Windows toolchain cannot follow those symbolic links. |
Aha... This is probably the problem:
That will need to be |
And this seems to be because the test list you are using specifies the Linux toolchain:
Unfortunately, you cannot re-use the Linux test lists for Cygwin. The test lists need |
It is, but only because the Windows cpp can't follow the symbolic links and cannot handle the POSIX-style include paths. |
Ah you are right. I'll see if I can fix that, we are unfortunately using -c for two different things in cibuild.sh and testbuild.sh so I'll have to sort that as well |
In my original test lists (https://bitbucket.org/nuttx/tools/src/master/buildtest/armlist.template) I specified the toolchain as CONFIG_ARMV7M_TOOLCHAIN_GNU_EABIx. I then changed the x to either W or L using sed. |
There are also errors in the sim build |
This is probably real code breakage. Again, it is a screw-up in include paths:
sim_wcap.c hasn't been build in ages. It is mixing Cygwin and Windows socket definitions. As I recall, this is a host file and, hence should be using all Windows definitions. The likelihood that this still works (even if built correctly) is low. I will look into this too. |
This was, I am pretty sure, introduced with commit: 8ce0ff5ce44416511be343c842c8d714687182cf with this change:
up_internal.h includes:
And netinet/in.h includes:
Which is where the collision error is introduced since up_wpcap.c includes winsock2.h already. The solution: Revert that single file change. |
@btashton PR apache/nuttx#1903 solves this build problem. |
Thanks Greg, I'll check it out shortly. I am getting my local windows environment back up to make this testing a little quicker. |
@patacongo I ran the Cygwin build locally and when I try to run the executable it crashes. Strace shows this, any idea what is going on? (nettest and nsh had the same result)
|
@patacongo I see this is mentioned in the README sorry. Hmm I take that back. It is called out in the README, but the fix does not seem to work. |
Signed-off-by: Brennan Ashton <[email protected]>
051ef3f
to
1efd200
Compare
sim:module sim:tcpblaster sim:dsptest sim:sotest all failed. A couple others failed as well but that was due to not having patch installed. I'll add that to the required Cygwin tools. The ARM builds all passed though which is good. 2.5hr is really not great. I'm wondering why this is sooo slow and if there is something we can do about it. |
68b6c91
to
7b9e06d
Compare
@@ -391,6 +420,9 @@ function run_builds { | |||
Linux) | |||
ncpus=`grep -c ^processor /proc/cpuinfo` | |||
;; | |||
Cygwin) | |||
ncpus=`grep -c ^processor /proc/cpuinfo` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no difference between Linux and Cygwin?
@@ -0,0 +1,5 @@ | |||
/arm/stm32f0l0g0,CONFIG_ARMV7M_TOOLCHAIN_GNU_EABIW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we remove CONFIG_ARMV7M_TOOLCHAIN_GNU_EABIx from all datalist?
This is currently very much a WIP. Some of what is in the ci workflow file is working around issues where
cibuild.sh
does not support the Windows runners yet and will need to be moved over.This also syncs the workflow file with the rest of the project.