-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[tools] remove --flutter-repo flag in flutter test
#3728
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.
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
Conversation
CONTRIBUTING.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FLUTTER_REPO_DIR ==> FLUTTER_ROOT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FLUTTER_ROOT is already taken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But shouldn't it point to the same place? I should have said, what about using FLUTTER_ROOT below instead of defining a new var?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this alias is defined in .bashrc or .profile this variable will be globally visible, and therefore you won't be able to have both a checkout of the repo and a normal installation of Flutter (e.g. from the alpha branch). If we use FLUTTER_ROOT it will cause all Flutter installations to point to the same location, which might result in surprising behavior.
FLUTTER_REPO_DIR would only point to a working checkout of Flutter, and it would not affect other installations of it.
I'm fine with switching over to FLUTTER_ROOT if the alternative behavior is what we actually want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this example is just illustrative of how someone might set up a script to test their checkout; I didn't intend to bikeshed on it too much :) Happy w/ how you have it now.
|
lgtm |
949a732 to
1076285
Compare
|
Maybe we should make |
|
I just use travis/test.sh personally. Sometimes I use a variant of test.sh that uses |
CONTRIBUTING.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't bother talking about this. People should just use test.sh and if they know that they can look in that file to figure out how to do individual subparts of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gone.
1076285 to
6d41b83
Compare
|
Just ran |
This option has a misleading name, is not used in any of our builds, not useful to our users, and its benefits can be easily gained using a shell alias.
Fixes #1939
/cc @Hixie @devoncarew