Skip to content

Conversation

@yjbanov
Copy link
Contributor

@yjbanov yjbanov commented May 4, 2016

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

CONTRIBUTING.md Outdated
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@devoncarew
Copy link
Contributor

lgtm

@yjbanov yjbanov force-pushed the remove-test-flutter-repo branch from 949a732 to 1076285 Compare May 4, 2016 17:15
@abarth
Copy link
Contributor

abarth commented May 4, 2016

Maybe we should make travis/test.sh robust enough to be run as a replacement by developers?

@Hixie
Copy link
Contributor

Hixie commented May 4, 2016

I just use travis/test.sh personally. Sometimes I use a variant of test.sh that uses --engine-debug but is otherwise unchanged.

CONTRIBUTING.md Outdated
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gone.

@yjbanov yjbanov force-pushed the remove-test-flutter-repo branch from 1076285 to 6d41b83 Compare May 4, 2016 20:51
@yjbanov
Copy link
Contributor Author

yjbanov commented May 4, 2016

Just ran travis/test.sh myself. Worked great. Could certainly use an environment variable for specifying the engine to run with.

@yjbanov yjbanov merged commit 39e741d into flutter:master May 4, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flutter test --flutter-repo only tests package:flutter

4 participants