New issue
Advanced search Search tips
Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment
link

Issue 917604: Isolated go client does not archive symlinked files

Reported by joshuaseaton@google.com, Dec 22 Project Member

Issue description

Example:
https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=e63e777e5ef714a67ffc0df7d9cb2561cf6f6b29
A symlinked file's source information is registered, but is unavailable for download.

See here for the symlink switch in the client:
https://cs.chromium.org/chromium/infra/go/src/go.chromium.org/luci/client/isolated/archive.go?l=137&rcl=a6b2dd30843b7ab64250cb44a736f8dbe721fb2b
If the client detects a symlink, it calls Symlink() to create a file object with only the link field populated; else it returns BasicFile(), a normal isolated File with a digest (and no link).

BasicFile() and Symlink() are defined here:
https://cs.chromium.org/chromium/infra/go/src/go.chromium.org/luci/common/isolated/isolated.go?l=68-82&rcl=6a43384a3adb5f64ea1b71f56cb7490250144c6a
 

Comment 1 by joshuaseaton@google.com, Dec 22

Proposal: creation of a similar SymlinkFile() func, like BasicFile() but also takes a linkname as a parameter.

If this is reasonable, should all Symlink() instances be moved over to SymlinkFile()?
https://cs.chromium.org/search/?q=isolated.Symlink%5C(+file:%5Einfra/go/src/go%5C.chromium%5C.org/luci/+package:%5Echromium$&type=cs

Comment 2 by mar...@chromium.org, Dec 23

Status: Assigned (was: Untriaged)
The problem is isolatedFile() and its calling function archive(), it needs to:

If the destination is outside the tree:
  If destination is a file:
    save the symlink destination as a file as the original symlink
  else if a directory:
    recurse in the directory as if it had been there original, instead of a symlink
else:
  all the destination as if it had been specified as an input

Comment 3 by joshuaseaton@google.com, Dec 23

Please clarify what is happening on the level of the File struct.
In your algorithm above, what File objects are being made?


More visually, right now a symlink entry in an isolated in the UI
looks like
---------------------------------------------------------------
|    file      | digest | mode | size |  symlink   | filetype |
---------------------------------------------------------------
| linkname.txt |        |      |      | source.txt |          |
---------------------------------------------------------------
It has no digest, mode, or size. Should it have these fields?
Moreover, what determines whether 'linkname.txt' is downloadable?

Comment 4 by mar...@chromium.org, Dec 23

If the destination is outside the tree:
  If destination is a file:
    File is a file
  else if a directory:
    File is not added at all, since the directory is walked into.
else:
  File is a symlink

Comment 5 by bugdroid1@chromium.org, Jan 10

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/f0368c7871cdb4400c990eecc8774a3fbcf0e1f4

commit f0368c7871cdb4400c990eecc8774a3fbcf0e1f4
Author: Joshua Seaton <joshuaseaton@google.com>
Date: Thu Jan 10 19:45:16 2019

[isolated] Follow symbolic links in files

Bug: 917604
Test: The following was done
```
mkdir a
echo 'foo' > a/source
mkdir b
ln -s ../a/source b/link
isolated archive -I https://isolateserver.appspot.com -namespace default-gzip -files $CWD/b:link
```
At HEAD: https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=dcdb41730f584e42fc101c11bd00ad7dfc5a10cd
With this change: https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=f54ad05e5f19414626c74218e808edcef11088dc

Change-Id: Iad484b29e1ac9edfdb3f2cfb3c9f2adff8506ba9
Reviewed-on: https://chromium-review.googlesource.com/c/1394248
Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org>
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/f0368c7871cdb4400c990eecc8774a3fbcf0e1f4/client/isolated/archive.go
[modify] https://crrev.com/f0368c7871cdb4400c990eecc8774a3fbcf0e1f4/client/isolated/archive_test.go

Comment 6 by bugdroid1@chromium.org, Jan 15

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/ef5a719a5a06ca80bb59ffea625670f4397142a9

commit ef5a719a5a06ca80bb59ffea625670f4397142a9
Author: Joshua Seaton <joshuaseaton@google.com>
Date: Tue Jan 15 03:31:35 2019

[archiver] Make PushDirectory tests table-driven

This will make future modifications to PushDirectory easier to test.

Bug: 917604
Change-Id: Ia3d14209ec5caaa4cb18a5655716789f341235fd
Reviewed-on: https://chromium-review.googlesource.com/c/1404260
Commit-Queue: Joshua Seaton <joshuaseaton@google.com>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>

[modify] https://crrev.com/ef5a719a5a06ca80bb59ffea625670f4397142a9/client/archiver/directory_test.go

Comment 7 by bugdroid1@chromium.org, Jan 15

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/5c78afc7db0efe3c70843bee7c3fd991ef29326c

commit 5c78afc7db0efe3c70843bee7c3fd991ef29326c
Author: Joshua Seaton <joshuaseaton@google.com>
Date: Tue Jan 15 03:37:45 2019

[isolated] Follow symbolic links in directories.

Bug: 917604
Test: The following was done
```
mkdir a
echo 'foo' > a/srcf
mkdir a/srcdir
echo 'bar' > a/srcdir/f
mkdir b
echo 'baz' > b/intreesrc
ln -s ../a/srcf b/linkf
ln -s ../a/srcdir b/linkdir
ln -s intreesrc b/intreelink

isolated archive -I https://isolateserver.appspot.com -namespace default-gzip -dirs $CWD:b
```
At HEAD: https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=f5f3a5991eab03d049d8eda6ed49ef393c46e7ec
With this change: https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=711b1dd90aa45ccb53fc5ebec86c2d37b40c914a

Change-Id: I4b11c9c079f95e43e5c5f5b80ebeaa84b5af7307
Reviewed-on: https://chromium-review.googlesource.com/c/1390262
Commit-Queue: Joshua Seaton <joshuaseaton@google.com>
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/5c78afc7db0efe3c70843bee7c3fd991ef29326c/client/archiver/directory.go
[modify] https://crrev.com/5c78afc7db0efe3c70843bee7c3fd991ef29326c/client/archiver/directory_test.go
[modify] https://crrev.com/5c78afc7db0efe3c70843bee7c3fd991ef29326c/client/internal/common/filesystem_view.go

Sign in to add a comment