Summary

[tfs] Allow overriding tf.cmd location

Review Request #7999 — Created Feb. 25, 2016 and submitted March 1, 2016, 12:12 p.m.

Information

Owner

bmartin

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

Allow an environment variable to specify location of tf.cmd for rbtools.
This will allow tf commands from visual studio 2013+ to work alongside
an installation of Team Explorer Everywhere.

Testing Done

Before:
C:\>rbt post -d
>>> RBTools 0.8 alpha 0 (dev)
>>> Python 2.7.11 (v2.7.11:6d1b6a68f775, Dec  5 2015, 20:32:19) [MSC v.1500 32 bit (Intel)]
>>> Running on Windows-10-10.0.10586
Checking location "%RBT_TF_PATH%\tf.cmd"
Caught OSError: [Error 2] The system cannot find the file specified


After:
C:\>rbt post -d
>>> RBTools 0.8 alpha 0 (dev)
>>> Python 2.7.11 (v2.7.11:6d1b6a68f775, Dec  5 2015, 20:32:19) [MSC v.1500 32 bit (Intel)]
>>> Running on Windows-10-10.0.10586
Checking location C:\Program Files (x86)\TEE-CLC-14.0.2\tf.cmd
Using location C:\Program Files (x86)\TEE-CLC-14.0.2\tf.cmd

Issues

Description	From	Last Updated
These shouldn't be one sentence per line.	brennie	Feb. 26, 2016, 2:38 p.m.
Perhaps this should be exposed as a .reviewboardrc option instead?	brennie	Feb. 26, 2016, 3:59 p.m.
Blank line between these.	brennie	Feb. 26, 2016, 2:37 p.m.
The config in __init__ is from load_config :) We'll also want to use options (also included above) instead, see my …	brennie	Feb. 26, 2016, 7:22 p.m.
I believe we'll want to add an option group to rbtools.commands.Command, similar to perforce_options circa line 435. That way, we …	brennie	Feb. 26, 2016, 7:22 p.m.
tf_locations.extend Otherwise, your array will look like ['my-custom-path', ['tf.cmd', '...']] which is not what you were intending.	brennie	Feb. 26, 2016, 7:22 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/clients/tfs.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tfs.py

rbtools/clients/tfs.py (Diff revision 1)

This environmental variable gets evaluated by the shell, when check_install gets run, right? Because since it's a raw string, I think Python won't do it.

bmartin Feb. 26, 2016, 12:39 a.m.

Yep, exactly. The same way the two installation locations are checked.

smith Feb. 26, 2016, 7:10 a.m.

Well, I mean, not the exact same way - there's no %ENV_VARIABLE% getting evaluated in the other cases! - but yeah, I'm satisfied. Thumbs-up.

bmartin Feb. 26, 2016, 11:36 a.m.

r'%programfiles(x86)%\Microsoft Visual Studio 12.0\Common7\IDE\tf.cmd',
r'%programfiles%\Microsoft Team Foundation Server 12.0\Tools\tf.cmd',

%programfiles(x86)% and %programfiles% are environment variables that, for the large majority of people, will resolve to C:\Program Files (x86) and C:\Program Files respectively.

C:\>echo %programfiles(x86)%
C:\Program Files (x86)

C:\>echo %programfiles%
C:\Program Files

Thanks for the review and the thumbs-up! ^_^

So this is my first submission, do I need another review, just commit, or do I create a master branch pull request?

Change Summary:

I dug in further and it turns out that variable expansion isn't currently working for win/tfs. Rather than switch check_install to run in the shell, I've added a call to have python expand the variables before checking the installation.

Testing Done:

~		Running "tf" uses tf.exe from the path and running "rbt" will first check for tf.cmd in the specified location before trying the path then the installation locations.
	~	Before:
	+
	+	`C:\>rbt post -d`
	+	>>> RBTools 0.8 alpha 0 (dev)
	+	>>> Python 2.7.11 (v2.7.11:6d1b6a68f775, Dec 5 2015, 20:32:19) [MSC v.1500 32 bit (Intel)]
	+	>>> Running on Windows-10-10.0.10586
	+	Checking location "%RBT_TF_PATH%\tf.cmd"
	+	Caught OSError: [Error 2] The system cannot find the file specified
	+
	+
	+
	+
	+	After:
	+
	+	`C:\>rbt post -d`
	+	>>> RBTools 0.8 alpha 0 (dev)
	+	>>> Python 2.7.11 (v2.7.11:6d1b6a68f775, Dec 5 2015, 20:32:19) [MSC v.1500 32 bit (Intel)]
	+	>>> Running on Windows-10-10.0.10586
	+	Checking location C:\Program Files (x86)\TEE-CLC-14.0.2\tf.cmd
	+	Using location C:\Program Files (x86)\TEE-CLC-14.0.2\tf.cmd
	+

Diff:

Revision 2 (+5 -2)

Show changes

rbtools/clients/tfs.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tfs.py



Tool: Pyflakes
Processed Files:
    rbtools/clients/tfs.py

rbtools/clients/tfs.py (Diff revision 2)

Show all issues

These shouldn't be one sentence per line.

rbtools/clients/tfs.py (Diff revision 2)

Show all issues

Perhaps this should be exposed as a .reviewboardrc option instead?

bmartin Feb. 26, 2016, 2:40 p.m.

I'm not sure about this. In general, .reviewboardrc contains the settings around a RB Repository and a local workspace of the same repo, right? However the installation location for Team Explorer Everywhere is very likely to be quite different from machine to machine (It's just an unzipped directory, there is no installer for it).

brennie Feb. 26, 2016, 2:42 p.m.

That is true, but we also support .reviewboardrc in higher up directories, so it seems like ~/.reviewboardrc would be a good place for this (or whatever the equivalent would be in windows with %HOME%\.reviewboardrc)

bmartin Feb. 26, 2016, 2:57 p.m.

Ahh, cool. I'll go ahead and make the change.

rbtools/clients/tfs.py (Diff revision 2)
Show all issues
```
Blank line between these.
```

Change Summary:

Fixed comment line lengths and spacing around if.

Diff:

Revision 3 (+6 -2)

Show changes

rbtools/clients/tfs.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tfs.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tfs.py

Change Summary:

Switched to pulling the cmd location from config rather than environment variables. Tested with no TF_CMD, one explicitly defining it, and one with an environment variable.

D:\GitHub\rbtools>more %APPDATA%\.reviewboardrc
OPEN_BROWSER = True
# TF_CMD = 'C:\\Program Files (x86)\\TEE-CLC-14.0.2\\tf.cmd'
# TF_CMD = '%programfiles(x86)%\\TEE-CLC-14.0.2\\tf.cmd'

The override should now work across platforms.

Diff:

Revision 4 (+17 -9)

Show changes

rbtools/clients/tfs.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tfs.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tfs.py

rbtools/clients/tfs.py (Diff revision 4)

Show all issues

The config in __init__ is from load_config :)

We'll also want to use options (also included above) instead, see my notes below.

BM

bmartin Feb. 26, 2016, 7:21 p.m.
```
Doh. fixed.
```

rbtools/clients/tfs.py (Diff revision 4)

Show all issues

I believe we'll want to add an option group to rbtools.commands.Command, similar to perforce_options circa line 435.

That way, we can also specify, e.g. --tf-path on the command line, and we can get a named options.

Then we can do:

if options.tf_cmd_path:
    tf_locations.append(options.tf_cmd_path)

BM

bmartin Feb. 26, 2016, 7:21 p.m.
```
Cool, switched.
```

rbtools/clients/tfs.py (Diff revision 4)

Show all issues

tf_locations.extend
Otherwise, your array will look like
['my-custom-path', ['tf.cmd', '...']]

which is not what you were intending.

bmartin Feb. 26, 2016, 7:21 p.m.

Sorry for the churn. My python-fu is rudimentary at best. ^_^

Change Summary:


Use the already loaded config
Allow command line specification of tf cmd.

Testing

rbt post -d --tf-cmd "C:\Program Files (x86)\TEE-CLC-14.0.2\tf.cmd"
Specification in %APPDATA%.reviewboardrc also work (with and without env variables).

Diff:

Revision 5 (+22 -9)

Show changes

	rbtools/clients/tfs.py
	rbtools/commands/__init__.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tfs.py
    rbtools/commands/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tfs.py
    rbtools/commands/__init__.py

Change Summary:

Safer access to tf_cmd option in case it's undefined.

Diff:

Revision 6 (+22 -9)

Show changes

	rbtools/clients/tfs.py
	rbtools/commands/__init__.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tfs.py
    rbtools/commands/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tfs.py
    rbtools/commands/__init__.py

Ship it!

Looks good! Thanks so much for doing this!

BM

bmartin March 1, 2016, 6:17 p.m.
```
Cool, thanks for the review! ^_^
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.7.x (847a34e)