Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2186263002:
luci-py: Refactor file writing code to allow file objects. (Closed)
Descriptionluci-py: Refactor file writing code to allow file objects.
This change refactors writing files out of the cache to support file like
objects as the source. This allows files to be extracted from archives or
directly written from a socket.
The change also makes sure that only read only files are linked
(otherwise modifications could corrupt the cache). It also makes sure that
read only files in the isolate actually have read only permissions.
* https://github.com/luci/luci-go/issues/9
* https://crbug.com/598990
BUG=598990
Committed: https://github.com/luci/luci-py/commit/4a9a641bd2314bd4b40607598187b0ac6305203a
Patch Set 1 : Removing arfile stuff #
Total comments: 4
Patch Set 2 : Fixes for review. #
Total comments: 2
Patch Set 3 : Fixing the fs.open usage. #Patch Set 4 : Rebase #
Dependent Patchsets: Messages
Total messages: 29 (23 generated)
Patchset #1 (id:1) has been deleted
maruel@chromium.org changed reviewers: + maruel@chromium.org
you can remove the fact that it was split out of another CL in the CL description, it's not worth committing this bit of history. please update the CL description to call out the change in file mode handling. https://codereview.chromium.org/2186263002/diff/20001/client/isolateserver.py File client/isolateserver.py (right): https://codereview.chromium.org/2186263002/diff/20001/client/isolateserver.py... client/isolateserver.py:241: srcfileobj = fs.open(srcfileobj, 'rb') You forget to close it. https://codereview.chromium.org/2186263002/diff/20001/client/isolateserver.py... client/isolateserver.py:255: link_mode = file_path.HARDLINK_WITH_FALLBACK else: link_mode = file_path.COPY and remove lines 248-249, I think it's clearer.
The CQ bit was checked by tansell@chromium.org 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 by full committers or 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.
Description was changed from ========== luci-py: Refactor file writing code to allow file objects. This change refactors writing files out of the cache to support file like objects as the source. This allows files to be extracted from archives or directly written from a socket. This was split out of adding support for archive files in http://crrev.com/2060983006 * https://github.com/luci/luci-go/issues/9 * https://crbug.com/598990 BUG=598990 ========== to ========== luci-py: Refactor file writing code to allow file objects. This change refactors writing files out of the cache to support file like objects as the source. This allows files to be extracted from archives or directly written from a socket. It also makes sure that read only files are actually have read only permissions. * https://github.com/luci/luci-go/issues/9 * https://crbug.com/598990 BUG=598990 ==========
Description was changed from ========== luci-py: Refactor file writing code to allow file objects. This change refactors writing files out of the cache to support file like objects as the source. This allows files to be extracted from archives or directly written from a socket. It also makes sure that read only files are actually have read only permissions. * https://github.com/luci/luci-go/issues/9 * https://crbug.com/598990 BUG=598990 ========== to ========== luci-py: Refactor file writing code to allow file objects. This change refactors writing files out of the cache to support file like objects as the source. This allows files to be extracted from archives or directly written from a socket. The change also makes sure that only read only files are linked (otherwise modifications could corrupt the cache). It also makes sure that read only files in the isolate actually have read only permissions. * https://github.com/luci/luci-go/issues/9 * https://crbug.com/598990 BUG=598990 ==========
I've updated the description to include the mode / readonly changes. https://codereview.chromium.org/2186263002/diff/20001/client/isolateserver.py File client/isolateserver.py (right): https://codereview.chromium.org/2186263002/diff/20001/client/isolateserver.py... client/isolateserver.py:241: srcfileobj = fs.open(srcfileobj, 'rb') On 2016/07/28 15:14:38, M-A Ruel wrote: > You forget to close it. Removed. https://codereview.chromium.org/2186263002/diff/20001/client/isolateserver.py... client/isolateserver.py:255: link_mode = file_path.HARDLINK_WITH_FALLBACK On 2016/07/28 15:14:38, M-A Ruel wrote: > else: > link_mode = file_path.COPY > > and remove lines 248-249, I think it's clearer. Done.
The CQ bit was checked by tansell@chromium.org 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 by full committers or 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.
lgtm with fix https://codereview.chromium.org/2186263002/diff/40001/client/tests/isolateser... File client/tests/isolateserver_test.py (right): https://codereview.chromium.org/2186263002/diff/40001/client/tests/isolateser... client/tests/isolateserver_test.py:253: isolateserver.putfile(fs.open(infile, 'rb'), cp, file_mode=0755) I'd prefer a with fs.open() here too to not leak these handles during the test, which also would break the test on Windows. lines 260 and 267
The CQ bit was checked by tansell@chromium.org 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/2186263002/diff/40001/client/tests/isolateser... File client/tests/isolateserver_test.py (right): https://codereview.chromium.org/2186263002/diff/40001/client/tests/isolateser... client/tests/isolateserver_test.py:253: isolateserver.putfile(fs.open(infile, 'rb'), cp, file_mode=0755) On 2016/07/28 15:53:23, M-A Ruel wrote: > I'd prefer a with fs.open() here too to not leak these handles during the test, > which also would break the test on Windows. > lines 260 and 267 Done.
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 tansell@chromium.org 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 tansell@chromium.org
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/2186263002/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== luci-py: Refactor file writing code to allow file objects. This change refactors writing files out of the cache to support file like objects as the source. This allows files to be extracted from archives or directly written from a socket. The change also makes sure that only read only files are linked (otherwise modifications could corrupt the cache). It also makes sure that read only files in the isolate actually have read only permissions. * https://github.com/luci/luci-go/issues/9 * https://crbug.com/598990 BUG=598990 ========== to ========== luci-py: Refactor file writing code to allow file objects. This change refactors writing files out of the cache to support file like objects as the source. This allows files to be extracted from archives or directly written from a socket. The change also makes sure that only read only files are linked (otherwise modifications could corrupt the cache). It also makes sure that read only files in the isolate actually have read only permissions. * https://github.com/luci/luci-go/issues/9 * https://crbug.com/598990 BUG=598990 Committed: https://github.com/luci/luci-py/commit/4a9a641bd2314bd4b40607598187b0ac6305203a ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://github.com/luci/luci-py/commit/4a9a641bd2314bd4b40607598187b0ac6305203a |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
