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.

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

x/build: windows builders and try-bots mingw is out of date #35006

Open
alexbrainman opened this issue Oct 19, 2019 · 10 comments

Comments

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Oct 19, 2019

Builders and try-bots have gcc 5.1 installed on them. According to https://sourceforge.net/projects/mingw-w64/files/ we could get gcc 8.1.

I discovered problem with our builders while fixing #23649. For example, current debug/pe tests fail, if gcc 8.1 is used, but pass fine on our builders. CL 197977 fixes broken debug/pe tests, but I cannot even verify it on builders / try-bots.

I propose we install gcc 8.1 so we can test #23649 fix, and keep it fixed. gcc 5.1 is many years old, we should use recent gcc on our builders.

I used these files from https://sourceforge.net/projects/mingw-w64/files/

https://sourceforge.net/projects/mingw-w64/files/Toolchains%20targetting%20Win64/Personal%20Builds/mingw-builds/8.1.0/threads-win32/seh/x86_64-8.1.0-release-win32-seh-rt_v6-rev0.7z

https://sourceforge.net/projects/mingw-w64/files/Toolchains%20targetting%20Win32/Personal%20Builds/mingw-builds/8.1.0/threads-win32/dwarf/i686-8.1.0-release-win32-dwarf-rt_v6-rev0.7z

amd64 and 386 versions. I am not sure, if I used correct versions, but they seems to work with my change. I am happy to use any alternative location / version - as long as it is recent. @mattn maybe you have suggestions which mingw version to use.

I also am not sure what to do about old gcc 5.1 installed on our builders. Perhaps we should have both old and new gcc versions installed onto our disk images. And then have different builders use different gcc versions. For better coverage. I noticed that golang.org/x/build/cmd/buildlet.windowsPath adds gcc bin directory to the path, perhaps we could adjust that code.

@bradfitz can you help?

Thank you.

Alex

@gopherbot gopherbot added this to the Unreleased milestone Oct 19, 2019
@gopherbot gopherbot added the Builders label Oct 19, 2019
@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Oct 19, 2019

I'm too busy to work on this unfortunately.

/cc @toothrot @dmitshur

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Oct 20, 2019

I'm too busy to work on this unfortunately.

No worries. I will try to have a go at this myself. When I have time.

Alex

@dmitshur dmitshur added the NeedsFix label Oct 21, 2019
@zx2c4

This comment has been minimized.

Copy link
Contributor

@zx2c4 zx2c4 commented Oct 23, 2019

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Oct 25, 2019

I'd recommend using the ones from https://musl.cc/ . They're up to date and work very well:

http://musl.cc/x86_64-w64-mingw32-native.zip
http://musl.cc/i686-w64-mingw32-native.zip

I tried running all.bat on the current tip against

http://musl.cc/x86_64-w64-mingw32-native.zip

and it all passes. While, if I do the same against

https://sourceforge.net/projects/mingw-w64/files/Toolchains%20targetting%20Win64/Personal%20Builds/mingw-builds/8.1.0/threads-win32/seh/x86_64-8.1.0-release-win32-seh-rt_v6-rev0.7z

it fails in debug/pe with error similar to #23649 (comment)

Why is that?

And, while I, probably, prefer your version of gcc, our builders should test against what most users use.

Alex

@zx2c4

This comment has been minimized.

Copy link
Contributor

@zx2c4 zx2c4 commented Oct 25, 2019

I'll CC @zv-io to talk about why the musl.cc compilers are better.

Usually those compilers are up to date and use recent MingW that fixes bugs.

And, while I, probably, prefer your version of gcc, our builders should test against what most users use.

We should direct users to using actually maintained compilers. Windows toolchains are a moving target, and those old ones have many problems.

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Oct 25, 2019

We should direct users to using actually maintained compilers.

I am all for making better tools available to Go users.

But we still need to fix bugs that Go users experience while using whatever tools they use. How do you propose we handle bugs like #23649 if we don't use same compilers as #23649 users?

Alex

@zx2c4

This comment has been minimized.

Copy link
Contributor

@zx2c4 zx2c4 commented Oct 25, 2019

But we still need to fix bugs that Go users experience while using whatever tools they use. How do you propose we handle bugs like #23649 if we don't use same compilers as #23649 users?

I have no interest adding hacks to Golang to work around known bugs in ancient compilers that aren't supported by Microsoft in the first place. It would be one thing if we supported MSVC and there was a bug there we have to deal with. But here we're talking about some random compilers people download from Sourceforge. I won't waste my time fighting with that junk. Rather, most of these things have been fixed upstream already, and @zv-io is nice enough to make builds available at musl.cc.

@zv-io

This comment has been minimized.

Copy link

@zv-io zv-io commented Oct 25, 2019

Hi, I maintain these.

I'd recommend using the ones from https://musl.cc/ . They're up to date and work very well:
http://musl.cc/x86_64-w64-mingw32-native.zip
http://musl.cc/i686-w64-mingw32-native.zip

I tried running all.bat on the current tip against

http://musl.cc/x86_64-w64-mingw32-native.zip

and it all passes. While, if I do the same against

https://sourceforge.net/projects/mingw-w64/files/Toolchains%20targetting%20Win64/Personal%20Builds/mingw-builds/8.1.0/threads-win32/seh/x86_64-8.1.0-release-win32-seh-rt_v6-rev0.7z

it fails in debug/pe with error similar to #23649 (comment)

Why is that?

I can't explain why the other toolchain causes the error, and the build log referenced by that comment is nonexistent. I'm having a hard time finding information about how those toolchains are built. I use the latest MinGW-W64 when I prepare each release, and provide recipes and build scripts for all releases.

And, while I, probably, prefer your version of gcc, our builders should test against what most users use.

There's #23649 (comment) which maybe someone else can advise on, but I'm going to agree with @zx2c4 that it's entirely a waste of time to figure out how to hack around bugs in broken toolchains. Further, not encouraging their use might reduce your maintenance burden, not add to it.

I'm guessing that "most users" won't complain about using up-to-date, reproducible toolchains that don't require JavaScript or advertisements to download. They even work on Windows XP.

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Oct 26, 2019

But here we're talking about some random compilers people download from Sourceforge.

This is what all Go users use. Including our builders (see https://github.com/golang/go/wiki/WindowsBuild ).

If we are to change this, then we should all agree what Mingw we use. Perhaps we should even provide the url in https://golang.org/doc/install - if there are many Mingw versions and they work differently, then we should be explicit there. And we should update our builders to run the same software.

I am fine using http://musl.cc if everyone agrees. @rsc and @ianlancetaylor ?

And we won't be fixing issues like #23649, because they are not reproducible with http://musl.cc . @mattn can you, please, verify? I used http://musl.cc/x86_64-w64-mingw32-native.zip

I can't explain why the other toolchain causes the error,

@zv-io thank you for taking time to comment here. The fix for #23649 is here

https://go-review.googlesource.com/c/go/+/197977/

And the gist of the fix is, it works around commit b295099 from git://git.code.sf.net/p/mingw-w64/mingw-w64 Does your Mingw build includes this commit? The commit (as I understand it) adds __imp___acrt_iob_func symbol. That breaks Go internal linker, because Go assumes that all symbols starting with __imp_ live in external DLLs. The bug does not happens with http://musl.cc - I did not investigate it enough to understand why.

Alex

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 26, 2019

Change https://golang.org/cl/203603 mentions this issue: Revert "cmd/link: use libmsvcrt.a during internal link"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.