Skip to content

Improve Windows build failure message#36845

Merged
zanderso merged 3 commits intoflutter:masterfrom
GroovinChip:master
Aug 1, 2019
Merged

Improve Windows build failure message#36845
zanderso merged 3 commits intoflutter:masterfrom
GroovinChip:master

Conversation

@GroovinChip
Copy link
Copy Markdown
Contributor

Description

This PR improves the build failure message for Windows build targets. It suggests that the user run flutter run -d windows -v to see a stack trace, and if that does not help the user solve the build failure it prompts the user to open an issue.

Related Issues

Contributes to, but does not fully address, #33583

Tests

This PR does not change any behavior.

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 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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@fluttergithubbot
Copy link
Copy Markdown
Contributor

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygine page and make sure this patch meets those guidelines before LGTMing.

/cc @dnfield

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 24, 2019
@GroovinChip
Copy link
Copy Markdown
Contributor Author

@jonahwilliams Review please? Can't do it officially as I'm not a contributor.

@jonahwilliams jonahwilliams self-requested a review July 24, 2019 19:14
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.

Please remove: please file an issue at https://github.com/flutter/flutter/issues/new/choose. Since this error could be anything from a flutter build error to a reconfigured app to a compilation error in platfrom code it doesn't really provide any more signal.

@GroovinChip
Copy link
Copy Markdown
Contributor Author

Done

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

@jonahwilliams
Copy link
Copy Markdown
Contributor

Thank you for the contribution @GroovinChip !

@GroovinChip
Copy link
Copy Markdown
Contributor Author

You're very welcome @jonahwilliams :D

@GroovinChip
Copy link
Copy Markdown
Contributor Author

GroovinChip commented Jul 24, 2019

To further the goal of making Windows build logs available in the console, would it make sense to take the stderr from the following code and print it?

build_windows.dart:

  try {
    process.stderr
      .transform(utf8.decoder)
      .transform(const LineSplitter())
      .listen(printError);
    process.stdout
      .transform(utf8.decoder)
      .transform(const LineSplitter())
      .listen(printTrace);
    result = await process.exitCode;
  } finally {
    status.cancel();
  }

stderr is defined in process.dart as:

  /**
   * Returns the standard error stream of the process as a [:Stream:].
   */
  Stream<List<int>> get stderr;

I image we could get the contents of process.stderr into a String and print that to the console?

@jonahwilliams
Copy link
Copy Markdown
Contributor

printError should already show up in the console above this message (if there was anything). It's possible we're dropping something on the floor before that. We would have to manually buffer to stderr output.

@GroovinChip
Copy link
Copy Markdown
Contributor Author

GroovinChip commented Jul 24, 2019

So, somewhere in here, then?

  final Process process = await processManager.start(<String>[
    buildScript,
    vcvarsScript,
    fs.path.basename(solutionPath),
    configuration,
  ], workingDirectory: fs.path.dirname(solutionPath));

I know that the buildScript is vs_build.bat but that simply calls vcvars64.bat as far as I can tell.

EDIT: @jonahwilliams Perhaps we can use the Message Task to get msbuild logs? We can update msbuild_utils.dart to use it.

@jonahwilliams
Copy link
Copy Markdown
Contributor

That process call should already be logging all of the stderr. But it might be the case that we're not running the msbuild with the right verbosity. Let's land this change now, since it is simpler, and go further down the rabbit hole separately.

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

@zanderso zanderso merged commit ced2078 into flutter:master Aug 1, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 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.

5 participants