Skip to content

[Windows] [GN] Windows GN Builds #191

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

Merged
merged 25 commits into from
Jan 8, 2019
Merged

[Windows] [GN] Windows GN Builds #191

merged 25 commits into from
Jan 8, 2019

Conversation

CallumIddon
Copy link
Contributor

This allows GN builds on Windows using the MSVC build tools. Currently just requesting feedback on changes before adding a post #161 patch.

This moves the main library header to include/flutter_desktop_embedding/windows and adds the flutter_desktop_embedding namespace to coincide with the Linux implementation.

The GLFW import has also been changed to coincide with the Linux implementation in preparation for including the files under common that already include GLFW with the Linux style imports.

The common files have been removed from the GN sources as they require jsoncpp to build. This patch is marked WIP as if it lands before #161 then a follow up patch would be required to allow #161 to build with GN. Once #161 has landed the common imports can be re-added as jsoncpp will be available.

Currently the example app cannot be built with GN as getting Visual Studio to build with the GN output would require more configurations. It would be more reasonable to add this later and just have 2 configurations, one for Visual Studio and one for GN, like the Linux builds instead of every combination of Static/Dynamic, Debug/Release, VS/GN. I have manually confirmed that the GN build does run correctly.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this! I won't have time for a detailed review until later this week (I left a few comments where I noticed things during my initial read-through), but in general this all looks really good.

Doing the plumbing to allow the example to build with the GN version in a follow-up makes sense to me, since there's already plenty in this PR.

@CallumIddon
Copy link
Contributor Author

CallumIddon commented Jan 1, 2019

This now also adds the common files to the GN build and removes the VS static library builds to address #192.

@CallumIddon CallumIddon changed the title [WIP] [Windows] [GN] Windows GN Builds [Windows] [GN] Windows GN Builds Jan 1, 2019
@stuartmorgan-g
Copy link
Collaborator

and removes the VS static library builds

As this is unrelated to supporting GN, it should be landed separately.

@CallumIddon
Copy link
Contributor Author

No longer addresses #192

<Filter>Source Files</Filter>
</None>
</ItemGroup>
</Project>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what is going on here. Any idea why git is marking the file as changed when they are exactly the same?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not at my computer so I can't verify at the moment, but my guess would be that the line endings changed.

@stuartmorgan-g
Copy link
Collaborator

Sorry for the churn from #194. That should make this PR slightly simpler though, since now there's even less divergence between the list of files that the shared BUILD.gn target are building. (E.g., you don't need to move the Windows embedder.h any more since Windows just uses glfw/embedder.h.)

@CallumIddon
Copy link
Contributor Author

This requires the run_dart_tool script you mentioned, other than that everything should be okay.

@stuartmorgan-g
Copy link
Collaborator

This requires the run_dart_tool script you mentioned, other than that everything should be okay.

I thought I could get to this today, but I didn't have much time and apparently writing a batch script to remove one argument and pass the rest through to another call (without it being broken for some cases) is surprisingly difficult. (I have the bash version done locally, as it's trivial.) Sorry for the delay!

@CallumIddon
Copy link
Contributor Author

No worries. I know how painful batch can be sometimes...

@clarkezone
Copy link
Collaborator

Agreed

@stuartmorgan-g
Copy link
Collaborator

stuartmorgan-g commented Jan 7, 2019

PR #210 will add run_dart_tool

(AFAICT the cases where arguments will not be passed correctly in that implementation are unlikely to matter, and at least one I found already wasn't being handled correctly by update_flutter_engine.bat, so I decided to go with something straightforward and then revisit if we ever need to pass, say, an "&" through it.)

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small comments, but otherwise this looks ready to go!

Thanks again for taking this on, and all the iteration! (Beyond getting GN up and running for Windows, it's also just great to see so much batch be replaced with Dart, which will be much easier to maintain.)

@stuartmorgan-g
Copy link
Collaborator

If everything else is good we should be ready to merge.

I was doing a final once-over and realized that I hadn't checked the Dart locally; there are a bunch of analyzer warnings, and I'm trying to keep the Dart code as warning-free as possible. I can clean them up tomorrow and then merge (or if you feel like doing that before then feel free; at a glance I think most will be straightforward, but avoid_relative_lib_imports suggests I misunderstood the lib/bin structure, so we'll just disable avoid_relative_lib_imports for now until I can revisit that).

@CallumIddon
Copy link
Contributor Author

@stuartmorgan not sure why travis is failing. I ran a Linux example build on a fresh clone with a fresh version on flutter as travis would do and had no problems, can you get the same result locally?

@stuartmorgan-g
Copy link
Collaborator

Sorry about that, I accidently broke trunk yesterday evening, and this branch's test ran before I landed the fix. I restarted it and it looks good now.

@stuartmorgan-g stuartmorgan-g merged commit ebe0af7 into google:master Jan 8, 2019
@CallumIddon CallumIddon deleted the windows-gn branch January 8, 2019 23:00
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants