[flutter_tools] Discover pubspec.yaml in parent directories#48548
[flutter_tools] Discover pubspec.yaml in parent directories#48548fluttergithubbot merged 5 commits intoflutter:masterfrom
Conversation
fluttergithubbot
added
the
tool
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
I really like this idea, and I think that 99% of the time it would work just right.
I'm a bit worried about the 1% of the time where I have a random nested Flutter project and now don't know which pubspec I'm getting where I used to get an error that I was in the wrong directory. Or if I (accidentally?) copy a pubspec.yaml into my $HOME/src/ directory and all of the sudden the tool treats any folder under that as a flutter project.
jonahwilliams
changed the title
|
I thought about that as well, but note that a failing |
| while (!globals.fs.isFileSync('pubspec.yaml')) { | ||
| final Directory nextCurrent = globals.fs.currentDirectory.parent; | ||
| if (nextCurrent == null || nextCurrent.path == globals.fs.currentDirectory.path) { | ||
| throw ToolExit(userMessages.flutterNoPubspec); |
There was a problem hiding this comment.
Maybe we could add something in here about which directories we traversed and failed to find?
| if (nextCurrent == null || nextCurrent.path == globals.fs.currentDirectory.path) { | ||
| throw ToolExit(userMessages.flutterNoPubspec); | ||
| } | ||
| globals.fs.currentDirectory = nextCurrent; |
There was a problem hiding this comment.
If this loop succeeds, it would probably help if we printed a status saying what directory we are using as the cwd.
There was a problem hiding this comment.
Only if we are not using the cwd we started with
There was a problem hiding this comment.
We should not ever print non-actionable messages, see https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#only-log-actionable-messages-to-the-console
There was a problem hiding this comment.
The action would be to change your cwd if something has gone wrong, right?
There was a problem hiding this comment.
Imagine, for example, the following directory structures:
src/
pubspec.yaml // put here last week when I didn't trust git to not lose it on me
flutter_app_1/
pubspec.yaml
android/
java_app_1/
// no pubspec.yaml
lib/
main.dart
One day I do flutter run in java_app_1 and to my surprise weird things happen. I would have found it really helpful to know that it was because the tool found a stray pubspec.yaml that I left laying around on my FS.
Alternatively, imagine I did flutter run -t ../lib/main.dart from the android directory. And imagine that, for some crazy reason, I have a ../../lib/main.dart present in my filesystem. The log message would have made it clearer which main.dart was actually getting used.
There was a problem hiding this comment.
Won't the dart process at least potentially be resolving relative paths, such as the -t option to flutter run?
There was a problem hiding this comment.
Sure, but those might be wrong anyway. At any rate, we're still strictly better than we were before:
- flutter run (in lib/): failed
- flutter run --target=main.dart (in lib/): failed
After:
- flutter run (in lib/): passed
- flutter run --target=main.dart (in lib/): failed
If this leads to confusion, we can always tweak the error messaging
There was a problem hiding this comment.
What's the output of flutter run --target=main.dart in lib/ with this patch?
There was a problem hiding this comment.
Target file "main.dart" not found.
There was a problem hiding this comment.
Okay, I'll back out my comment on non-actionable message :) . Like you suggested, we status log to the new CWD if it is updated, then the error message above is more interpretable
| fail('Expect exception'); | ||
| } catch (e) { | ||
| expect(e, isInstanceOf<ToolExit>()); | ||
| expect(e.toString(), contains('Target file "lib/main.dart" not found.')); |
There was a problem hiding this comment.
should we also expect to find the log told us what directory we ended up in? Or am I misunderstanding what this test is actually setting up?
5 participants
If there is no pubspec.yaml in the current directory, walk upwards until we either find one or run out of parents.
Fixes #48117