Return named output files if they're symlinks

Return named output files if they're symlinks

https://github.com/luci/luci-py/issues/299
Review-Url: https://codereview.chromium.org/2928153002
Committed: https://github.com/luci/luci-py/commit/510cde54a95487852db6a74d80c22eaeae25d350

[aludwin, 2017-06-09 16:46:30]

aludwin@google.com changed reviewers:
+ maruel@chromium.org

[M-A Ruel, 2017-06-09 17:00:41]

https://codereview.chromium.org/2928153002/diff/1/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/2928153002/diff/1/client/run_isolated.py#newc...
client/run_isolated.py:337: if os.path.islink(infile):
use fs.islink() otherwise this throws on Windows.

https://codereview.chromium.org/2928153002/diff/1/client/run_isolated.py#newc...
client/run_isolated.py:339: file_path.readable_copy(outfile,
os.path.realpath(infile))
use fs.copy2()
I don't think realpath() is necessary.

https://codereview.chromium.org/2928153002/diff/1/client/tests/run_isolated_t...
File client/tests/run_isolated_test.py (right):

https://codereview.chromium.org/2928153002/diff/1/client/tests/run_isolated_t...
client/tests/run_isolated_test.py:789: '  os.symlink(realpath, sys.argv[2])\n'
this will fail on Windows, I'd prefer a new test so you can disable the test
case on Windows for now.

[aludwin, 2017-06-09 19:37:32]

https://codereview.chromium.org/2928153002/diff/1/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/2928153002/diff/1/client/run_isolated.py#newc...
client/run_isolated.py:337: if os.path.islink(infile):
On 2017/06/09 17:00:40, M-A Ruel wrote:
> use fs.islink() otherwise this throws on Windows.

Done.

https://codereview.chromium.org/2928153002/diff/1/client/run_isolated.py#newc...
client/run_isolated.py:339: file_path.readable_copy(outfile,
os.path.realpath(infile))
On 2017/06/09 17:00:40, M-A Ruel wrote:
> use fs.copy2()
> I don't think realpath() is necessary.

Done.

https://codereview.chromium.org/2928153002/diff/1/client/tests/run_isolated_t...
File client/tests/run_isolated_test.py (right):

https://codereview.chromium.org/2928153002/diff/1/client/tests/run_isolated_t...
client/tests/run_isolated_test.py:789: '  os.symlink(realpath, sys.argv[2])\n'
On 2017/06/09 17:00:41, M-A Ruel wrote:
> this will fail on Windows, I'd prefer a new test so you can disable the test
> case on Windows for now.

This code is guarded by 'if sys.platform.startswith("linux")' - shouldn't that
be good enough?

[M-A Ruel, 2017-06-09 19:43:17]

Forgot to git cl upl again.

[M-A Ruel, 2017-06-09 20:16:22]

lgtm

https://codereview.chromium.org/2928153002/diff/20001/client/tests/run_isolat...
File client/tests/run_isolated_test.py (right):

https://codereview.chromium.org/2928153002/diff/20001/client/tests/run_isolat...
client/tests/run_isolated_test.py:784: 'import os\n'
OMG it's not sorted.

https://codereview.chromium.org/2928153002/diff/20001/client/tests/run_isolat...
client/tests/run_isolated_test.py:787: '  realpath = os.path.join(os.getcwd(),
"contents_of_symlink")\n'
realpath = os.path.abspath("contents_of_symlink")
but strictly speaking, it is not necessary as a relative path would be just
fine.

[aludwin, 2017-06-12 12:59:11]

https://codereview.chromium.org/2928153002/diff/20001/client/tests/run_isolat...
File client/tests/run_isolated_test.py (right):

https://codereview.chromium.org/2928153002/diff/20001/client/tests/run_isolat...
client/tests/run_isolated_test.py:784: 'import os\n'
On 2017/06/09 20:16:22, M-A Ruel wrote:
> OMG it's not sorted.

Done.

https://codereview.chromium.org/2928153002/diff/20001/client/tests/run_isolat...
client/tests/run_isolated_test.py:787: '  realpath = os.path.join(os.getcwd(),
"contents_of_symlink")\n'
On 2017/06/09 20:16:22, M-A Ruel wrote:
> realpath = os.path.abspath("contents_of_symlink")
> but strictly speaking, it is not necessary as a relative path would be just
> fine.

Fixed. Because "sys.argv[2]" can be in another directory (and is, in this test),
we need an absolute path here, not a relative one.

[aludwin, 2017-06-12 13:00:05]

The CQ bit was checked by aludwin@google.com

[aludwin, 2017-06-12 13:00:07]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/2928153002/#ps40001
(title: "Fixed import order")

[commit-bot: I haz the power, 2017-06-12 13:00:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aludwin, 2017-06-12 13:01:19]

Description was changed from

==========
Return named output files if they're symlinks

BUG=299
==========

to

==========
Return named output files if they're symlinks

https://github.com/luci/luci-py/issues/299
==========

[commit-bot: I haz the power, 2017-06-12 13:03:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497272405153770,
"parent_rev": "876754ce7917bedfd6c72e6fc02a86a94ee15c36", "commit_rev":
"510cde54a95487852db6a74d80c22eaeae25d350"}

[commit-bot: I haz the power, 2017-06-12 13:03:09]

Description was changed from

==========
Return named output files if they're symlinks

https://github.com/luci/luci-py/issues/299
==========

to

==========
Return named output files if they're symlinks

https://github.com/luci/luci-py/issues/299
Review-Url: https://codereview.chromium.org/2928153002
Committed:
https://github.com/luci/luci-py/commit/510cde54a95487852db6a74d80c22eaeae25d350
==========

[commit-bot: I haz the power, 2017-06-12 13:03:10]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-py/commit/510cde54a95487852db6a74d80c22eaeae25d350