Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844063005:
Add option to collapse symlinks in isolate.py (Closed)
DescriptionAdd option to collapse symlinks in isolate.py
BUG=
Review-Url: https://codereview.chromium.org/2844063005
Committed: https://github.com/luci/luci-py/commit/c2e0ff0ab9e23d9eb58b58418d8aa3e8b01d147b
Patch Set 1 #Patch Set 2 : fix isolate test #Patch Set 3 : fix space #
Total comments: 10
Patch Set 4 : Address comments #
Total comments: 4
Patch Set 5 : Address nits #Patch Set 6 : fix tests #
Total comments: 1
Patch Set 7 : fix isolateserver test #Patch Set 8 : more tests fix #Patch Set 9 : Add -L #
Dependent Patchsets: Messages
Total messages: 44 (31 generated)
The CQ bit was checked by kjlubick@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
kjlubick@google.com changed reviewers: + maruel@chromium.org
+Tim and Michael Kevin wants to package CIPD packages as isolated files to test something on their RPi setup because CIPD package extract is currently quite slow on an RPi. The snag is that the CIPD packages symlinks was getting in the way so they just want to inline all files as if they were real inodes. For now they would use the python client but I want to make sure you are in the loop. https://codereview.chromium.org/2844063005/diff/40001/client/isolate.py File client/isolate.py (right): https://codereview.chromium.org/2844063005/diff/40001/client/isolate.py#newco... client/isolate.py:482: extra_variables, blacklist, ignore_broken_items, collapse_symlinks=False): would you mind not making it a default arg? https://codereview.chromium.org/2844063005/diff/40001/client/isolate.py#newco... client/isolate.py:1131: default=False, not needed https://codereview.chromium.org/2844063005/diff/40001/client/isolated_format.py File client/isolated_format.py (right): https://codereview.chromium.org/2844063005/diff/40001/client/isolated_format.... client/isolated_format.py:313: filepath, prevdict, read_only, algo, collapse_symlinks=False): no default please https://codereview.chromium.org/2844063005/diff/40001/client/isolated_format.... client/isolated_format.py:329: algo: Hashing algorithm used. update https://codereview.chromium.org/2844063005/diff/40001/client/isolated_format.... client/isolated_format.py:344: filestats = None not needed
https://codereview.chromium.org/2844063005/diff/40001/client/isolate.py File client/isolate.py (right): https://codereview.chromium.org/2844063005/diff/40001/client/isolate.py#newco... client/isolate.py:482: extra_variables, blacklist, ignore_broken_items, collapse_symlinks=False): On 2017/04/27 at 17:44:02, M-A Ruel wrote: > would you mind not making it a default arg? Done. https://codereview.chromium.org/2844063005/diff/40001/client/isolate.py#newco... client/isolate.py:1131: default=False, On 2017/04/27 at 17:44:02, M-A Ruel wrote: > not needed Done. https://codereview.chromium.org/2844063005/diff/40001/client/isolated_format.py File client/isolated_format.py (right): https://codereview.chromium.org/2844063005/diff/40001/client/isolated_format.... client/isolated_format.py:313: filepath, prevdict, read_only, algo, collapse_symlinks=False): On 2017/04/27 at 17:44:02, M-A Ruel wrote: > no default please Done. https://codereview.chromium.org/2844063005/diff/40001/client/isolated_format.... client/isolated_format.py:329: algo: Hashing algorithm used. On 2017/04/27 at 17:44:02, M-A Ruel wrote: > update Done. https://codereview.chromium.org/2844063005/diff/40001/client/isolated_format.... client/isolated_format.py:344: filestats = None On 2017/04/27 at 17:44:02, M-A Ruel wrote: > not needed Done.
lgtm with nits https://codereview.chromium.org/2844063005/diff/60001/client/isolate.py File client/isolate.py (right): https://codereview.chromium.org/2844063005/diff/60001/client/isolate.py#newco... client/isolate.py:1131: help='Treat any symlinks as if they were the normal underlying file.') no trailing dot https://codereview.chromium.org/2844063005/diff/60001/client/isolated_format.py File client/isolated_format.py (right): https://codereview.chromium.org/2844063005/diff/60001/client/isolated_format.... client/isolated_format.py:312: def file_to_metadata( this fits 80 cols
The CQ bit was checked by kjlubick@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2844063005/diff/60001/client/isolate.py File client/isolate.py (right): https://codereview.chromium.org/2844063005/diff/60001/client/isolate.py#newco... client/isolate.py:1131: help='Treat any symlinks as if they were the normal underlying file.') On 2017/04/27 at 17:58:49, M-A Ruel wrote: > no trailing dot Done. https://codereview.chromium.org/2844063005/diff/60001/client/isolated_format.py File client/isolated_format.py (right): https://codereview.chromium.org/2844063005/diff/60001/client/isolated_format.... client/isolated_format.py:312: def file_to_metadata( On 2017/04/27 at 17:58:49, M-A Ruel wrote: > this fits 80 cols Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Luci-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35c9660fbe342210)
The CQ bit was checked by kjlubick@google.com
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/2844063005/#ps100001 (title: "fix tests")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Luci-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35c96b1840817510)
tansell@chromium.org changed reviewers: + tansell@chromium.org
I'm not strongly opposed to this patch but it does feels very much like this is being solved in the wrong place and the logic could grow to become quite complicated (IE I want this symlink expanded but not this one). Couldn't we just fix CIPD to resolve the symlinks on extraction? Also, if this is just needed for testing, you can use cp or rsync to do this "flattening" for you in a much nicer way. cp -Lr ~/toflatten ~/flattened rsync -L ~/toflatten ~/flattened Tim 'mithro' Ansell https://codereview.chromium.org/2844063005/diff/100001/client/isolate.py File client/isolate.py (right): https://codereview.chromium.org/2844063005/diff/100001/client/isolate.py#newc... client/isolate.py:1131: help='Treat any symlinks as if they were the normal underlying file') It might be nice to have the alias "-L" to be consistent with rsync / cp.
The CQ bit was checked by kjlubick@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Luci-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35cd5fca9ce8d710)
The CQ bit was checked by kjlubick@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Luci-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35cd8fdca5fb0710)
The CQ bit was checked by kjlubick@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kjlubick@google.com
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/2844063005/#ps160001 (title: "Add -L")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Luci-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35cdd0dfe6c0b710)
The CQ bit was checked by kjlubick@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493392218345590,
"parent_rev": "46c1328bdc6575a5ff09cce3b8e9dae609fcfcc9", "commit_rev":
"c2e0ff0ab9e23d9eb58b58418d8aa3e8b01d147b"}
Message was sent while issue was closed.
Description was changed from ========== Add option to collapse symlinks in isolate.py BUG= ========== to ========== Add option to collapse symlinks in isolate.py BUG= Review-Url: https://codereview.chromium.org/2844063005 Committed: https://github.com/luci/luci-py/commit/c2e0ff0ab9e23d9eb58b58418d8aa3e8b01d147b ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://github.com/luci/luci-py/commit/c2e0ff0ab9e23d9eb58b58418d8aa3e8b01d147b
Message was sent while issue was closed.
On 2017/04/28 at 06:13:52, tansell wrote: > I'm not strongly opposed to this patch but it does feels very much like this is being solved in the wrong place and the logic could grow to become quite complicated (IE I want this symlink expanded but not this one). > > Couldn't we just fix CIPD to resolve the symlinks on extraction? > > Also, if this is just needed for testing, you can use cp or rsync to do this "flattening" for you in a much nicer way. > > cp -Lr ~/toflatten ~/flattened > rsync -L ~/toflatten ~/flattened > > Tim 'mithro' Ansell It's not just for testing - the production code would want to eliminate the cipd symlinks. I agree it may be nicer to have this option in CIPD and not here, but that was a bigger change than I wanted to wade into. > https://codereview.chromium.org/2844063005/diff/100001/client/isolate.py > File client/isolate.py (right): > > https://codereview.chromium.org/2844063005/diff/100001/client/isolate.py#newc... > client/isolate.py:1131: help='Treat any symlinks as if they were the normal underlying file') > It might be nice to have the alias "-L" to be consistent with rsync / cp. Done |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
