Made et compilation errors relative to the CWD #56177

gaaclarke · 2024-10-28T18:15:49Z

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

loic-sharma · 2024-10-28T20:12:52Z

I worry this solution might be brittle. Would it be possible to have et invoke the compiler from the correct working directory to get the desired output?

gaaclarke · 2024-10-28T20:25:36Z

I worry this solution might be brittle. Would it be possible to have et invoke the compiler from the correct working directory to get the desired output?

We're already invoking the ninja command from the src directory, which happens to be the CWD in my case and the paths are still wrong. I tried to supply a relative path to the -C argument to see if we got different results but we didn't.

If you are concerned about the regex being brittle:

The output of gcc is well document and duplicated by clang, that's not going to change without breaking tons of tools (including vscode)
The worst case scenario is that the output matches the output we had before this change.

zanderso · 2024-10-28T20:28:20Z

tools/pkg/engine_build_configs/lib/src/build_config_runner.dart

    return min(multiplier * cores, 1000);
  }();

+  static final RegExp _gccRegex =


This library should probably not have too many smarts in it. This logic should probably go in the event handler in et somewhere around https://github.com/flutter/engine/blob/main/tools/engine_tool/lib/src/build_utils.dart#L156.

All messages aren't routed through the eventHandler today. Your proposal would require that to be the case. I would expect printing proper paths would be fundamental to BuildRunner and not something every client of BuildRunner should set up.

All messages aren't routed through the eventHandler today.

What's missing? Looking below, _ninjaProgress returns true only after it sends a line of output to the event handler, and when it returns false, there's an early return that also skips the path rewriting.

printing proper paths would be fundamental

This library doesn't really "print" anything. That's the thinking behind the event handling part of the API. Clients of this API get to have an opinion about what the output seen by users looks like. If there's some data that clients need that isn't getting sent to the event handler, then we should fix that.

What's missing? Looking below, _ninjaProgress returns true only after it sends a line of output to the event handler, and when it returns false, there's an early return that also skips the path rewriting.

_ninjaProgress is parsing the output from ninja (like this PR does) and selectively forwards it on to the event handler. In our case it doesn't forward it on since it isn't a progress message. That's why modifying the line had to happen in BuildRunner.

Clients of this API get to have an opinion about what the output seen by users looks like.

Today they don't, the output just gets forward to stdout without their opinion. I suspect you think everything that is getting printed out is going through et which isn't the case. Given the choice to print wrong paths or correct paths we don't have anyone that wants the wrong paths so I don't think it's worth making that optional or something where a user should express their opinion. In my view this is a bug and not a missing extension point where users can change the behavior.

Oops. Yeah, you're right. I misread the code, and I see what you're saying now.

Here is my non-blocking advice: Similar to how progress information from ninja is parsed out and handed to the client as a RunnerProgress event, add a function like _ninjaError or _compilerError that parses out compiler messages (errors/warnings/notes) and hands them to the client with a RunnerError event. This way et will have the option of collecting/displaying compiler messages however it likes. Other use cases might be adding color/emoji/ascii art, writing to a json file, etc.

So I agree with Zach's (ultimate) position, but also agree it should be non-blocking - this is a strict improvement, is tested, and is small in scope. The most I would ask is a TODO/GH issue tracking improving this by uplifting it as a _xError, but not as a blocker for landing this (positive) change.

matanlurey · 2024-10-29T17:35:33Z

tools/pkg/engine_build_configs/lib/src/build_config_runner.dart

    return min(multiplier * cores, 1000);
  }();

+  static final RegExp _gccRegex =


So I agree with Zach's (ultimate) position, but also agree it should be non-blocking - this is a strict improvement, is tested, and is small in scope. The most I would ask is a TODO/GH issue tracking improving this by uplifting it as a _xError, but not as a blocker for landing this (positive) change.

matanlurey · 2024-10-29T17:36:31Z

tools/pkg/engine_build_configs/lib/src/build_config_runner.dart

+
+  /// Converts relative [path], who is relative to [dirPath] to a relative path
+  /// of the `CWD`.
+  static String _makeRelative(String path, String dirPath) {


I'm curious what this does meaningfully diferent than using the optional parameter from in p.relative: https://pub.dev/documentation/path/latest/path/relative.html?

from is confusing, it replaces the CWD part of the equation not were the path is relative to.

Ack, thanks!

matanlurey · 2024-10-29T17:36:57Z

tools/pkg/engine_build_configs/lib/src/build_config_runner.dart

+
+  /// Takes a [line] from compilation and makes the path relative to `CWD` where
+  /// the paths are relative to [outDir].
+  static String fixGccPaths(String line, String outDir) {


Should this be @visibleForTesting if that is the intent?

I omit that when the thing that is made visible is a pure function. Its visibility and usage can't hurt anything. I reserve @VisibleForTesting for things I want to be private but can't because of testing and using them in production can invalidate state. Happy to change it if that doesn't make sense.

It's less of a "can't hurt anything" and more "uh oh, we're now using this in other parts of the repo unexpectedly".

Let's make this @visibleForTesting the same way we'd make the function private if we could (this is package build_engine_config, not package gcc_stdio), if that makes sense. LGTM assuming we do that.

…ine#56177)

…157833) flutter/engine@999797a...57ed5d3 2024-10-29 [email protected] [Flutter GPU] Fix MSAA sample size and HostBuffer alignment. (flutter/engine#56218) 2024-10-29 [email protected] Change default TileMode for blur ImageFilter objects to null (flutter/engine#55552) 2024-10-29 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Reland: disable AHBs on devices that were upgraded to 29. (#56213)" (flutter/engine#56220) 2024-10-29 [email protected] Made et compilation errors relative to the CWD (flutter/engine#56177) 2024-10-29 [email protected] [Impeller] Reland: disable AHBs on devices that were upgraded to 29. (flutter/engine#56213) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

fixes flutter#157735 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Made et compilation errors relative to the CWD

47de069

gaaclarke force-pushed the relative-et branch from 4650d10 to 47de069 Compare October 28, 2024 18:19

added test

bc428e9

gaaclarke requested review from matanlurey and zanderso October 28, 2024 18:45

gaaclarke marked this pull request as ready for review October 28, 2024 18:45

gaaclarke requested a review from loic-sharma October 28, 2024 19:56

zanderso reviewed Oct 28, 2024

View reviewed changes

matanlurey suggested changes Oct 29, 2024

View reviewed changes

gaaclarke mentioned this pull request Oct 29, 2024

[et] start forwarding compilation errors to et from BuildRunner flutter/flutter#157816

Closed

added todo

f5bdcbf

gaaclarke requested a review from matanlurey October 29, 2024 17:49

matanlurey approved these changes Oct 29, 2024

View reviewed changes

added visibleForTesting

2ddf3c4

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 29, 2024

auto-submit bot merged commit 2f04ae8 into flutter:main Oct 29, 2024
28 checks passed

engine-flutter-autoroll mentioned this pull request Oct 29, 2024

Roll Flutter Engine from 999797a2f690 to 8b2c8501daa1 (3 revisions) flutter/flutter#157832

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 29, 2024

2f04ae8b8 Made et compilation errors relative to the CWD (flutter/eng…

be9fca3

…ine#56177)

engine-flutter-autoroll mentioned this pull request Oct 29, 2024

Roll Flutter Engine from 999797a2f690 to 57ed5d338e7e (5 revisions) flutter/flutter#157833

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 29, 2024

2f04ae8b8 Made et compilation errors relative to the CWD (flutter/eng…

62e38eb

…ine#56177)

Made et compilation errors relative to the CWD #56177

Made et compilation errors relative to the CWD #56177

Uh oh!

Conversation

gaaclarke commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

loic-sharma commented Oct 28, 2024

Uh oh!

gaaclarke commented Oct 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gaaclarke commented Oct 28, 2024 •

edited

Loading