Check for BUILD file for bazel projects only #4735
Conversation
src/io/flutter/run/MainFile.java
Outdated
| return dir.isDirectory() && ( | ||
| dir.findChild("pubspec.yaml") != null || dir.findChild(".packages") != null); | ||
| dir.findChild("pubspec.yaml") != null || | ||
| (WorkspaceCache.getInstance(project).isBazel() && dir.findChild("BUILD") != null) || |
There was a problem hiding this comment.
checking for a BUILD file could be fragile. Instead ,it would be better to check whether the directory is under the bazel root directory. I think that is the equivalent of what isAppDir is doing externally.
There was a problem hiding this comment.
That sounds reasonable, but how do we define the bazel root directory? Is it related to what we're doing in Workspace, e.g. findContainingWorkspaceFile or ProjectRootManager.getInstance(p).getContentRoots()?
There was a problem hiding this comment.
findContainingWorkspaceFile will give you a breadcrumb to the answer. If you go back up the relative path from that directory to google3/ you will be in the right place.
There was a problem hiding this comment.
Can this (i.e. asking for the "type" of workspace) be part of the flutter_tool interface? We are not able to customize the plugin easily in g3 but we are able to customize the tooling.
There was a problem hiding this comment.
To be clear. We are actually extremely robust on determining whether a project is bazel by checking for the existence of dart/config/intellij-plugins/flutter.json. This bug is a case of code being written that forgets that not all Dart projects have pubspec.yaml or .packages file. A solution would be to audit the code base for all checks for pubspec.yaml and replace them with a helper call that is specialized for bazel workspaces.
One advantage of the current system is we don't have to make users configure the flutter sdk directory manually for bazel projects as we are able to determine it automatically. If we relied on specifying a flutter sdk directory we would have a chicken and egg problem as a user would have to have configured flutter before we could ask it whether a particular directory is a bazel or vanilla project.
There was a problem hiding this comment.
Understood.
Audit the code base for all checks for pubspec.yaml and replace them with a helper call that is specialized for bazel workspaces.
SGTM. I implemented a similar strategy for dartdoc where all "package" related operations go through an overridable class called PackageInfo rather than random code doing package processing. This also has the added advantage that what the plugin needs from pubspec.yaml is well defined and testable rather than being scattered all over the code.
There was a problem hiding this comment.
Using a PackageInfo class sounds like a clean solution. Filed #4738 to track cleaning up this tech debt.
We removed a check for BUILD to fix #4692 but this caused problems for internal projects.