Skip to content

Allow a custom dart version in the environment in computeExclusiveDevDependencies #158370

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.

Sign up for GitHub

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

Jump to bottom
Closed
yiiim wants to merge 3 commits into from
Closed

Allow a custom dart version in the environment in computeExclusiveDevDependencies #158370

yiiim wants to merge 3 commits into from

Conversation

@yiiim
Copy link
Member

@yiiim yiiim commented Nov 8, 2024 •
edited
Loading

computeExclusiveDevDependencies does not use Artifact when executing the dart command, which can cause exceptions when the dart version in the environment variable is different from the dart version used to execute the flutter command. This directly prevents the flutter run command from running in the main channel when the environment variable is not set.

Pre-launch Checklist

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

@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 8, 2024
@yiiim yiiim requested a review from matanlurey November 8, 2024 10:57
@yiiim
fix tools
77439e6 
fix flutter tool
@yiiim yiiim force-pushed the fix_flutter_tools branch from ab84e78 to 77439e6 Compare November 8, 2024 12:21
@matanlurey matanlurey changed the title Fix flutter tool issues Allow a custom dart version in the environment in computeExclusiveDevDependencies Nov 8, 2024
matanlurey
matanlurey reviewed Nov 8, 2024
View reviewed changes
Copy link
Contributor

@matanlurey matanlurey left a comment

Choose a reason for hiding this comment

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

In general LGTM.

Tests will need to be tweaked a bit for this to pass everywhere, but I can push a few commits up and help you land this. Thanks for noticing and thanks for writing a test!

@matanlurey matanlurey self-assigned this Nov 8, 2024
@matanlurey
Copy link
Contributor

matanlurey commented Nov 8, 2024

There is one failing test still:

plugin_test_macos` on a PR:

Unable to find a built dart sdk at: "/Volumes/Work/s/w/ir/x/t/flutter_devicelab_plugin_test.Nu6VxW/engine/out/foo/dart-sdk" or a prebuilt dart sdk at: "/Volumes/Work/s/w/ir/x/t/flutter_devicelab_plugin_test.Nu6VxW/engine/flutter/prebuilts/macos-x64/dart-sdk"

It looks like this test, which is only run on macOS, creates a simulated local-engine environment, but does so incorrectly (that is, it only adds a single artifact and misses things like the pre-built Dart SDK):

flutter/dev/devicelab/lib/tasks/plugin_tests.dart

Lines 125 to 138 in 2efdc74

if (buildTarget == 'macos') {
// When using a local engine, podhelper.rb will search for a "macos-"
// directory within the FlutterMacOS.xcframework, so create a dummy one.
Directory(
path.join(buildDir.path, 'FlutterMacOS.xcframework/macos-arm64_x86_64'),
).createSync(recursive: true);
// Clean before regenerating the config to ensure that the pod steps run.
await inDirectory(Directory(app.rootPath), () async {
await evalFlutter('clean');
});
await app.build(buildTarget, configOnly: true, localEngine: buildDir);
}
}

It looks like this is quite old, and we never observed a problem because it happened to be that nothing in the Flutter tool path for macos used Artifacts, but it does now :(. I could make work around this by falling back to the global dart instance but I'd obviously prefer not to do this.

@matanlurey
Copy link
Contributor

matanlurey commented Nov 8, 2024

@yiiim After talking with folks internal here, I'm going to have to put this PR on hold and refactor the base assumptions to use the Pub class. I'll make sure your use case works as part of that refactor.

@matanlurey matanlurey marked this pull request as draft November 8, 2024 20:58
@flutter-dashboard
Copy link

flutter-dashboard bot commented Nov 8, 2024

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

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

@matanlurey matanlurey mentioned this pull request Nov 8, 2024
Closed
@yiiim yiiim mentioned this pull request Nov 11, 2024
Closed
@matanlurey matanlurey mentioned this pull request Nov 12, 2024
Merged
@matanlurey
Copy link
Contributor

matanlurey commented Nov 12, 2024

@yiiim I believe #158524 will allow a custom Dart version to be used.

@yiiim yiiim closed this Nov 13, 2024
auto-submit bot pushed a commit that referenced this pull request Nov 13, 2024
@matanlurey
Move dart pub deps call to <Pub>.deps and use it accordingly (#15…
2d1a3ef 
…8524)

Closes #158396, closes #158370.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matanlurey matanlurey matanlurey left review comments

Assignees

@matanlurey matanlurey

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yiiim @matanlurey