Conversation
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
or just use it because it'll make CI more stable.
Let'd do this. We can suppress warning in native compiler configs in the buildroot for targets that we don't control and patch the rest.
@rmacnak-google for thoughts on wiring this up in the Dart buildroot as well.
| 'llvm-build', 'Release+Asserts')) | ||
|
|
||
|
|
||
| def GetDiaDll(): |
There was a problem hiding this comment.
What is this DLL for?
There was a problem hiding this comment.
This is what Chromium does upstream to build with. Without this, the build fails because it can't find expected symbols. Don't really have more details beyond that.
| }, | ||
|
|
||
| # TODO(fxb/4443): Remove this when Fuchsia can provide the Windows Clang Toolchain | ||
| 'src/third_party/llvm-build/Release+Asserts': { |
There was a problem hiding this comment.
Can't this just be //build/toolchain like for all other platforms?
There was a problem hiding this comment.
Right now the buildroot is looking for it here. We can patch buildroot for this, but I think for now this makes it clearer we're not using Fuchsia clang. WDYT?
tools/gn
Outdated
| gn_args['is_debug'] = args.unoptimized | ||
| gn_args['android_full_debug'] = args.target_os == 'android' and args.unoptimized | ||
| gn_args['is_clang'] = not sys.platform.startswith(('cygwin', 'win')) | ||
| gn_args['is_clang'] = True |
There was a problem hiding this comment.
We have a --clang/--no-clang argument (with clang being the default). We should honor it here, so that someone who wants to build with VS instead of clang can do so.
tools/dia_dll.py
Outdated
| if bool(int(os.environ.get('DEPOT_TOOLS_WIN_TOOLCHAIN', '1'))): | ||
| dia_path = os.path.join(win_sdk_dir, '..', 'DIA SDK', 'bin', 'amd64') | ||
| else: | ||
| if 'GYP_MSVS_OVERRIDE_PATH' not in os.environ: |
There was a problem hiding this comment.
Do we have existing use of this environment variable? If not, we should name it something else, rather than use a legacy name based on a build system that Flutter has never used.
There was a problem hiding this comment.
I believe this is used by depot_tools. I'd rather avoid creating a new one that might not play well with the existing buildroot/depot_tools
tools/dia_dll.py
Outdated
| if bool(int(os.environ.get('DEPOT_TOOLS_WIN_TOOLCHAIN', '1'))): | ||
| dia_path = os.path.join(win_sdk_dir, '..', 'DIA SDK', 'bin', 'amd64') | ||
| else: | ||
| if 'GYP_MSVS_OVERRIDE_PATH' not in os.environ: |
There was a problem hiding this comment.
Nit: prefer "if (foo) ... else ..." to "if (!foo) ... else ..." constructions in general, as they read more naturally (due to the lack of double-negative).
| # Bump after VC updates. | ||
| DIA_DLL = { | ||
| '2013': 'msdia120.dll', | ||
| '2015': 'msdia140.dll', |
There was a problem hiding this comment.
Do we support 2013 or 2015?
There was a problem hiding this comment.
I don't think so, but the upstream script had it and I figured it may be helpful when trying to figure out how to add new ones.
There was a problem hiding this comment.
Well, the upstream script has it because they have supported these versions in the past. I was thinking leave the map construction here even though it's not currently needed, to make it easy to change in the future, but remove the old entries that we will never use.
tools/gn
Outdated
| gn_args['is_debug'] = args.unoptimized | ||
| gn_args['android_full_debug'] = args.target_os == 'android' and args.unoptimized | ||
| gn_args['is_clang'] = not sys.platform.startswith(('cygwin', 'win')) | ||
| gn_args['is_clang'] = True |
There was a problem hiding this comment.
We have a --clang/--no-clang argument (with clang being the default). We should honor it here, so that someone who wants to build with VS instead of clang can do so.
| # Bump after VC updates. | ||
| DIA_DLL = { | ||
| '2013': 'msdia120.dll', | ||
| '2015': 'msdia140.dll', |
There was a problem hiding this comment.
Well, the upstream script has it because they have supported these versions in the past. I was thinking leave the map construction here even though it's not currently needed, to make it easy to change in the future, but remove the old entries that we will never use.
|
Why? Building everything with Clang is an anti-goal. Toolchain diversity should be maintained. |
|
@rmacnak-google - this patch will still use MSVC for linking. GOMA is dropping support for MSVC. Building without GOMA is very slow. We also run into lots of little idiosyncrosies between MSVC and Clang. And anecdotally, Chrome has been happier with building on Clang vs MSVC .. or so I've heard third hand. |
|
@rmacnak-google See flutter/flutter#16256 and flutter/flutter#48831
Based on which Flutter principle?
Not at the expense of actually being able to build the product we want to build. |
That just seems like sound engineering principle to me. And in this case, it has helped us as our dependencies have mostly been fine about compiling using Clang on Windows (specifically the TUs that have only every been compiled on Windows).
Totally. And I am going to hazard a guess and say Ryan didn't mean we should build only with MSVC on Windows. My take on toolchain diversity is that we should stay away from compiler specific extensions unless absolutely necessary. This doesn't mean we should go out of our way and wire up various compilers on our buildbots. Not ensuring we build on MSVC does mean that issues may creep into the codebase that may cause problems on MSVC. But those ought to be solvable. As long as we can say that the compiler would be supported if we found a maintainer for it, thats fine by me. If however, we get to a point where we have to say that compiling with any compiler other than Clang is not possible because we ended up using Clang specific extensions, that would be unfortunate. And I don't think this has happened yet. |
I don't really see how, honestly. The toolchain is a tool; it's a means to an end. I could just as easily say "using a single build system is an anti-goal", but I suspect you wouldn't consider that an obvious engineering principle. Also, you may want to talk to this person, who argued that we shouldn't even use different versions of the same toolchain if possible ;)
Agreed, but AFAIK being able to do non-standard things was never an argument for switching our default to clang on Windows.
This patch doesn't remove support for compiling Windows with MSVC, so it doesn't seem like that was @rmacnak-google's objection. |
|
I think @chinmaygarde was arguing in support of your points @stuartmorgan :) IMO, if we provide a way for contributors to get the toolchain and build on their OS, that's a good thing. We have historically had problems where contributors without access to Windows or with only access to Windows run into trouble due to idiosyncracies of MSVC's C++ implementation. If we can remove that as a hurdle, that's a good thing. But the primary driver is still around infra support - GOMA is dropping MSVC support, and it's become incredibly flakey in the last few weeks. That slows the whole team down, particularly when we need to get something through urgently for a fix downstream. We're not removing support for MSVC entirely, but we're no longer commiting to testing it. That, of course, is only true if I can get this patch working again... :D |
|
@stuartmorgan: Consider that we may be in contentious agreement here. The toolchain diversity is exactly what allowed us to pull a stunt like this one. If our dependencies had mandated that their Windows TUs could only be compiled with MSVC, this patch would be hosed. Clang makes sense for us because we get to use GOMA and makes sense for Dart because the support burden is likely not significant. That is fine, everyone gets to do what suits their goals the best. If you argue that toolchain diversity must be maintained by ensuring we use all possible compilers on our buildbots, thats not pragmatic. But no one is arguing for that. OTOH, if you argue that toolchain diversity must be maintained by ensuring we don't use Clang specific extensions, that is entirely reasonable and achievable. Viewed through this lens, I see no contention in the discussion here. In the past we had no choice but to support both toolchains, now we just need to be more careful. |
|
The latest issue on this is that if I try to compile using the Windows libcxx, I get failures in BoringSSL related to https://boringssl.googlesource.com/boringssl/+/7b935937b18215294e7dbf6404742855e3349092/crypto/hrss/hrss.c#64. It looks like it's not strictly MSVC that's the issue, but the Windows libcxx implementation as well. Unfortunately, I'm also having trouble getting things going with Clang's libcxx. I'll keep probing at that though. |
|
Are the issues you are seeing with clang 8? I didn't hit errors there with clang 11. |
|
Capturing from Discord discussion, the version I was testing with: |
|
Updated the PR to use the version of clang that works for me, but it looks like it's breaking the |
|
@stuartmorgan Are you still working on this? I was able to build more targets using the Clang 11 you posted to CIPD. However, I had to adding additional warnings suppressions as well as an error where std::atomicstd::mutex* was not trivially constructible. I am attempting to work past this ATM. If you are not working on this still, I can attempt to take it to completion. |
|
Nevermind, the warnings were non-fatal. I suppose it is just the atomic mutex thing. |
|
I was going to take another look next week to figure out what the remaining bot errors are. Feel free to send me reviews for any fixes you make before then though. |
5 participants
This uses a build of Clang 8.0.0 on Windows, fetched from Chromium.
It adds a script similar to what Chromium does for copying msdia*.dll to the right spot.
This works locally, but spouts tons of warnings from Dart, libpng, angle, etc. We could soften this for now by not making clang on by default, or just use it because it'll make CI more stable.