Skip to content

Switch Windows to CMake#60629

Merged
stuartmorgan-g merged 29 commits intoflutter:masterfrom
stuartmorgan-g:windows-cmake
Jul 6, 2020
Merged

Switch Windows to CMake#60629
stuartmorgan-g merged 29 commits intoflutter:masterfrom
stuartmorgan-g:windows-cmake

Conversation

@stuartmorgan-g
Copy link
Copy Markdown
Contributor

@stuartmorgan-g stuartmorgan-g commented Jun 30, 2020

Description

Switches Window builds from Visual Studio solution file + vcxproj to CMake.

Advantages:

  • Allows multi-target plugin builds without needing a new custom solution for dependency management.
  • Uses the same build system as Linux (although with a different generator, so there are differences),
    reducing the number of build systems Flutter developers need to know by one.
    • By the same token, reduces the number of build systems we need to maintain as well.
  • Solves the problem of toolchain version being hard-coded in projects, which would have made it
    difficult to move the ecosystem forward to new tool versions.
  • Due to being text-based, it's easier for people not familiar with VS to use, and easier to manage in
    version control.

Disadvantages:

  • Adds a layer of complexity to the build itself, since it's still using VS, but now has an
    additional CMake step.
  • The flow for people using VS is not as good. It now requires building once via the command
    line, then opening the solution in the build directory. The build-and-run of the primary target won't
    work, since there's no way to add the copy step in the right place with CMake (it doesn't support
    setting VS custom build steps, which is how that was done before), so developers using the
    VS workflow will need to 'build solution' then run the correct target.

Related Issues

Fixes #52753

Tests

I added the following tests: Updated all VS-specific tests to expect/use CMake instead.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 30, 2020
@stuartmorgan-g stuartmorgan-g marked this pull request as draft June 30, 2020 19:22
@stuartmorgan-g
Copy link
Copy Markdown
Contributor Author

Bonus advantage: no more .bat files 😁

// included by the main CMakeLists.txt, so relative paths must be relative to
// that file's directory.
final String makefileDirPath = project.windows.cmakeFile.parent.absolute.path;
final path.Context cmakePathContext = path.Context(style: path.Style.posix);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

interesting, this uses posix paths on windows too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, CMake always uses posix-style paths. That's what the use of TO_CMAKE_PATH in the other generated file is for, but here it was easier to just convert it before writing it.

(The reason for all the escaped Windows paths in the other file is that those aren't used by CMake, but passed through back to the tool script, so I want them to stay untouched.)

Copy link
Copy Markdown
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

This is fantastic, so much deleted code 😆

The analysis error is caused by ToT being red, should be fixed by #60638

'-B',
buildDir.path,
'-G',
'Visual Studio 16 2019',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: maybe put this in a constant, with some docs on where exactly it comes from, like "we only support this particular sdk, see doctor blah blah".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

It hadn't occurred to me that this may mean that the next major version of VS won't actually work since the generator is hard-coded; I don't know if these values are forward-compatible. But since this is in the tool, not the ecosystem of plugins and projects, we can easily add switching logic here later if that turns out to be the case. (Unlike the requirement in the old method of having a hard-coded toolchain version in the project file, which was one of the things that really worried me about the ecosystem longer term.) I added a comment to that effect on the constant.

@stuartmorgan-g stuartmorgan-g marked this pull request as ready for review June 30, 2020 22:26
@stuartmorgan-g stuartmorgan-g merged commit 4b12050 into flutter:master Jul 6, 2020
@stuartmorgan-g stuartmorgan-g deleted the windows-cmake branch July 6, 2020 19:59
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
* First pass at CMake files; untested

* First pass of adding CMake generation logic on Windows

* Misc fixes

* Get bundling working, start incoprorating CMake build into tool

* Fix debug, exe name.

* Add resources

* Move cmake.dart

* Rip out all the vcxproj/solution plumbing

* Fix plugin cmake generation

* Build with cmake rather than calling VS directly

* Adjust Windows plugin template to match standard header directory structure

* Pass config selection when building

* Partially fix multi-config handling

* Rev template version

* Share the CMake generation instead of splitting it out

* VS build/run cycle works, with slightly awkward requirement to always build all

* Update manifest

* Plugin template fixes

* Minor adjustments

* Build install as part of build command, instead of separately

* Test cleanup

* Update Linux test for adjusted generated CMake approach

* Plugin test typo fix

* Add missing stub file for project test

* Add a constant for VS generator
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decide on Windows desktop build tooling

4 participants