-
Notifications
You must be signed in to change notification settings - Fork 496
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
Add GN, google's metabuilder for Ninja. #2099
Conversation
So you would use cygwin python/ninja to build mingw v8? |
@lazka Correct, I have a PR for adding V8 to vcpkg (visual c++) at microsoft/vcpkg#12687 You'll notice that depot tools is not used on that PR, I have already successfully built v8 using mingw64, but I am missing gn for a full "native" build. The patch has merged upstream by the way 😁 |
GN is the metabuilder used to build chromium and v8.
Is it possible to convert this package to mingw one? |
@Biswa96 It is possible, but why would you do that? ninja is msys, unless you want to redistribute gn you only need it at build time. I have not tested whether these changes are sufficient for native mingw builds, my guess would be they are not, since there is a distinct path for Posix and Windows in the code and the code guards targeting MinGW may be stale and require more than adding some ifdefs here and there. |
MinGW packages are native to Windows OS. MSYS2 and Cygwin are compatible POSIX layer. So, MSYS2 programs are somewhat slower than native one. If a package can be compiled as native one then it will be preferred. But it would be better if Windows native build is supported by upstream project. |
How much slower is slower? GN is like cmake, you run it once, takes a couple of seconds and then the heavy lifting is done by ninja. Anyway, I see your point, there is msys ninja and mingw ninja, so how about I do both? |
So, mingw builds with just this patch, there's some warnings but it builds, however there seems to be a bug in mingw-w64-x86_64-ninja, for some reason the following rule deletes all input files and the static library: rule alink_thin seems to be a problem with the "rm -f $out" command since removing that command fixes the build. MSYS ninja does what it is supposed to do. |
There is no downside about building a package both for mingw and msys. But main goal of msys2 project is to build native programs. msys2 package is required when the program relies on POSIX functions and there is no way to convert it to Windows native. I tried to build the gn program in mingw mode but need some code changes. gn already supports Windows platform, so no significant change is required. Can you join in gitter to discuss more? |
Sure, I've never use gitter before, I just joined with github so ping me there or add me to a discussion. |
This is how I got it to build assuming you've cloned upstream and have a MINGW64 console already at the root of the repo: ./build/gen.py --platform=mingw |
That would be great, most of the warnings are due to GN redefining some macros in windows_types.h so they don't have to include windows.h, other than that I saw some pointer casting warnings, these can be fixed or silenced, however the bug in native mingw ninja is what worries me most. |
I don't know how to send pull request there. I follow all the steps but it does not appear in review webpage. If I send the changes can you add it there? |
Sure, send me patch. |
So, what else is needed on this one to get merged? |
If gn works as MinGW package then this can be removed. |
Since this is now part of the v8 built itself I'm closing this. As @Biswa96 we try to keep msys packages at a minimum and prefer native builds. If the need for this arises in the future feel free to re-open though and we can have another look. |
GN is the metabuilder used to build chromium and v8.
This is part of an effort to also provide scripts to build native mingw v8.
The msys support patch is under review at google at:
https://gn-review.googlesource.com/c/gn/+/9660
if/once the patch gets merged, it can be removed, but since a specific tag is being checked out, there should be no conflicts with upstream even if the patch is merged.