Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add symlink info to file metadata #153

Merged
merged 13 commits into from
Jul 14, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ github.com/bazelbuild/remote-apis v0.0.0-20190507145712-5556e9c6153f h1:KLZodm2m
github.com/bazelbuild/remote-apis v0.0.0-20190507145712-5556e9c6153f/go.mod h1:9Y+1FnaNUGVV6wKE0Jdh+mguqDUsyd9uUqokalrC7DQ=
github.com/bazelbuild/remote-apis v0.0.0-20191104140458-e77c4eb2ca48 h1:bgj+Oufa8F4rCHe/8omhml7cBlg3VmNhF66ed1vT2Bw=
github.com/bazelbuild/remote-apis v0.0.0-20191104140458-e77c4eb2ca48/go.mod h1:9Y+1FnaNUGVV6wKE0Jdh+mguqDUsyd9uUqokalrC7DQ=
github.com/bazelbuild/remote-apis v0.0.0-20200624085034-0afc3700d177 h1:JeDdP1ZsFOMctOzhcwYfLkGaLT8Nzxc1AXkXKUrWbAw=
github.com/boombuler/barcode v1.0.0 h1:s1TvRnXwL2xJRaccrdcBQMZxq6X7DvsMogtmJeHDdrc=
github.com/boombuler/barcode v1.0.0/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8=
github.com/bradfitz/go-smtpd v0.0.0-20170404230938-deb6d6237625 h1:ckJgFhFWywOx+YLEMIJsTb+NV6NexWICk5+AMSuz3ss=
Expand Down Expand Up @@ -111,6 +112,7 @@ github.com/golang/protobuf v1.3.2 h1:6nsPYzhq5kReh6QImI3k5qWzO4PEbvbIW2cwSfR/6xs
github.com/golang/protobuf v1.3.2/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/golang/protobuf v1.3.3 h1:gyjaxf+svBWX08ZjK86iN9geUJF0H6gp2IRKX6Nf6/I=
github.com/golang/protobuf v1.3.3/go.mod h1:vzj43D7+SQXF/4pzW/hwtAqwc6iTitCiVSaWz5lYuqw=
github.com/golang/protobuf v1.4.2 h1:+Z5KGCizgyZCbGh1KZqA0fcLLkwbsjIzS4aV2v7wJX0=
github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db h1:woRePGFeVFfLKN/pOkfl+p/TAqKOfFu+7KPlMVpok/w=
github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q=
github.com/gonum/blas v0.0.0-20181208220705-f22b278b28ac h1:Q0Jsdxl5jbxouNs1TQYt0gxesYMU4VXRbsTlgDloZ50=
Expand Down
2 changes: 1 addition & 1 deletion go/pkg/filemetadata/cache_posix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (

func TestExecutableCacheLoad(t *testing.T) {
c := NewSingleFlightCache()
filename, err := createFile(t, true, "")
filename, err := createFile(t, &testFileParams{executable: true})
if err != nil {
t.Fatalf("Failed to create tmp file for testing digests: %v", err)
}
Expand Down
6 changes: 3 additions & 3 deletions go/pkg/filemetadata/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ var (

func TestSimpleCacheLoad(t *testing.T) {
c := NewSingleFlightCache()
filename, err := createFile(t, false, "")
filename, err := createFile(t, &testFileParams{})
if err != nil {
t.Fatalf("Failed to create tmp file for testing digests: %v", err)
}
Expand Down Expand Up @@ -45,7 +45,7 @@ func TestSimpleCacheLoad(t *testing.T) {

func TestCacheOnceLoadMultiple(t *testing.T) {
c := NewSingleFlightCache()
filename, err := createFile(t, false, "")
filename, err := createFile(t, &testFileParams{})
if err != nil {
t.Fatalf("Failed to create tmp file for testing digests: %v", err)
}
Expand Down Expand Up @@ -75,7 +75,7 @@ func TestCacheOnceLoadMultiple(t *testing.T) {

func TestLoadAfterChangeWithoutValidation(t *testing.T) {
c := NewSingleFlightCache()
filename, err := createFile(t, false, "")
filename, err := createFile(t, &testFileParams{})
if err != nil {
t.Fatalf("Failed to create tmp file for testing digests: %v", err)
}
Expand Down
34 changes: 27 additions & 7 deletions go/pkg/filemetadata/filemetadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,25 @@ import (
"github.com/bazelbuild/remote-apis-sdks/go/pkg/digest"
)

// SymlinkMetadata contains details if the given path is a symlink.
type SymlinkMetadata struct {
Target string
IsInvalid bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: IsDangling is a more informative name, now that I think about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

// Metadata contains details for a particular file.
type Metadata struct {
Digest digest.Digest
IsExecutable bool
Err error
Symlink *SymlinkMetadata
}

// FileError is the error returned by the Compute function.
type FileError struct {
IsDirectory bool
IsNotFound bool
IsInvalidSymlink bool
Err error
IsDirectory bool
IsNotFound bool
Err error
}

// Error returns the error message.
Expand All @@ -41,11 +47,25 @@ func isSymlink(filename string) (bool, error) {
func Compute(filename string) *Metadata {
md := &Metadata{Digest: digest.Empty}
file, err := os.Stat(filename)
if isSym, _ := isSymlink(filename); isSym {
md.Symlink = &SymlinkMetadata{}
if err != nil {
md.Err = &FileError{Err: err}
md.Symlink.IsInvalid = true
return md
}
dest, err := os.Readlink(filename)
if err != nil {
// This should never happen given that we have verified |filename| is a symlink.
md.Err = &FileError{Err: err}
md.Symlink.IsInvalid = true
return md
}
md.Symlink.Target = dest
}

if err != nil {
fe := &FileError{Err: err}
if s, err := isSymlink(filename); err == nil && s {
fe.IsInvalidSymlink = true
}
if os.IsNotExist(err) {
fe.IsNotFound = true
}
Expand Down
174 changes: 159 additions & 15 deletions go/pkg/filemetadata/filemetadata_test.go
Original file line number Diff line number Diff line change
@@ -1,37 +1,55 @@
package filemetadata

import (
"crypto/rand"
"encoding/hex"
"errors"
"io/ioutil"
"os"
"path/filepath"
"testing"

"github.com/bazelbuild/remote-apis-sdks/go/pkg/digest"
"github.com/google/go-cmp/cmp"
)

func TestCompute(t *testing.T) {
type testFileParams struct {
contents string
executable bool
}

// Used as a type enum
type testDirParams struct {
}

func TestComputeFiles(t *testing.T) {
tests := []struct {
name string
contents string
executable bool
name string
*testFileParams
}{
{
name: "empty",
contents: "",
name: "empty",
testFileParams: &testFileParams{
contents: "",
},
},
{
name: "non-executable",
contents: "bla",
name: "non-executable",
testFileParams: &testFileParams{
contents: "bla",
},
},
{
name: "executable",
contents: "foo",
executable: true,
name: "executable",
testFileParams: &testFileParams{
contents: "foo",
executable: true,
},
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
filename, err := createFile(t, tc.executable, tc.contents)
filename, err := createFile(t, tc.testFileParams)
if err != nil {
t.Fatalf("Failed to create tmp file for testing digests: %v", err)
}
Expand Down Expand Up @@ -66,10 +84,95 @@ func TestComputeDirectory(t *testing.T) {
}
}

func createFile(t *testing.T, executable bool, contents string) (string, error) {
type testSymlinkCreationResult struct {
symlink string
target string
}

func (sc *testSymlinkCreationResult) cleanup() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If everything's under temp, why do you need to clean it up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually to follow

defer os.RemoveAll(filename)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow... Do you mean in the next patch? I meant to simplify as instead of cleaning up manually, create everything under a TempDir directory -- pass this directory to everything that creates files or symlinks if you need to reuse it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the confusion. My point is that the existing file tests are doing this clean up manually (even if those filenames are temporary and unique), therefore I followed that for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, right, now I wonder why I did that :-) But sure, let's keep it, then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, i actually removed the Clean() method..

os.RemoveAll(sc.symlink)
os.RemoveAll(sc.target)
}

func TestComputeSymlinks(t *testing.T) {
tests := []struct {
name string
target interface{} // If unspecified, this is an invalid symlink
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some cleaner options for doing this:

  1. What we usually do is make the test struct a union of the test parameters. There is nothing wrong with having an IsDirectory option, or an IsExecutable option that only applies to files and not directories.
  2. If you really want to have a single field of variable type, the golang pattern is to create a common interface (say, CreateSymlink) and have your types implement it.
    I slightly prefer option 2 in this case, because I suspect that if your createSymlink is split into two or three functions (you can create a different type for a dangling symlink), it will look much simpler.

Copy link
Contributor Author

@k-ye k-ye Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, actually i split this into three tests now. Sorry for mixing the logic before, hopefully it now gets simpler?

What we usually do is make the test struct a union of the test parameters.

Ack. I created testFileParams mostly because symlink tests also need this. But now it seems that we can get rid of it and flatten the test struct again. Let me know which way do you prefer!

BTW, I prepended all the test structs with test, in order to prevent it from being accidentally used in the non-test part of the package. Is this too verbose?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think flattening things out will be simplest. Also, I don't think the rest of the package can use the test code, at least not if compiled with Bazel (not sure how go build works), so no need to prefix with test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, removed testFileParams

}{
{
name: "valid",
target: &testFileParams{
contents: "bla",
},
},
{
name: "valid-executable",
target: &testFileParams{
contents: "executable",
executable: true,
},
},
{
name: "invalid-file",
},
{
name: "symlink-dir",
target: &testDirParams{},
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
symlinkResult, err := createSymlink(t, tc.target)
if err != nil {
t.Fatalf("Failed to create tmp symlink for testing digests: %v", err)
}
defer symlinkResult.cleanup()

symlinkPath := symlinkResult.symlink
got := Compute(symlinkPath)

if tc.target == nil {
if got.Err == nil || !got.Symlink.IsInvalid {
t.Errorf("Compute(%v) should fail because the symlink is invalid", symlinkPath)
}
if got.Symlink.Target != "" {
t.Errorf("Compute(%v) should fail because the symlink is invalid, got target: %s", symlinkPath, got.Symlink.Target)
}
return
}

if _, ok := tc.target.(*testDirParams); ok {
if fe, ok := got.Err.(*FileError); !ok || !fe.IsDirectory {
t.Errorf("Compute(%v).Err = %v, want FileError{IsDirectory:true}", symlinkPath, got.Err)
}
return
}

if got.Err != nil {
t.Errorf("Compute(%v) failed. Got error: %v", symlinkPath, got.Err)
}

fileParams := tc.target.(*testFileParams)
want := &Metadata{
Symlink: &SymlinkMetadata{
Target: symlinkResult.target,
IsInvalid: false,
},
Digest: digest.NewFromBlob([]byte(fileParams.contents)),
IsExecutable: fileParams.executable,
}

if diff := cmp.Diff(want, got); diff != "" {
t.Errorf("Compute(%v) returned diff. (-want +got)\n%s", symlinkPath, diff)
}
})
}
}

func createFile(t *testing.T, fp *testFileParams) (string, error) {
t.Helper()
perm := os.FileMode(0666)
if executable {
if fp.executable {
perm = os.FileMode(0766)
}
tmpFile, err := ioutil.TempFile(os.TempDir(), "")
Expand All @@ -83,8 +186,49 @@ func createFile(t *testing.T, executable bool, contents string) (string, error)
return "", err
}
filename := tmpFile.Name()
if err = ioutil.WriteFile(filename, []byte(contents), os.ModeTemporary); err != nil {
if err = ioutil.WriteFile(filename, []byte(fp.contents), os.ModeTemporary); err != nil {
return "", err
}
return filename, nil
}

func createSymlink(t *testing.T, target interface{}) (*testSymlinkCreationResult, error) {
t.Helper()

invalidTarget := target == nil
if invalidTarget {
// Create a temporary fake target so that os.Symlink() can work.
target = &testFileParams{
contents: "transient",
}
}
targetPath, err := func() (string, error) {
switch tgt := target.(type) {
case *testFileParams:
return createFile(t, tgt)
case *testDirParams:
return ioutil.TempDir(os.TempDir(), "")
}
return "", errors.New("Unknown target type")
}()
if err != nil {
return nil, err
}

randBytes := make([]byte, 16)
rand.Read(randBytes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplifying: pass the symlink name as a parameter, so you don't need to create it and return it. You can base it on the name of the test case, if you don't want to add another variable. That way, you don't need the testSymlinkCreationResult at all, you just return the file name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx! Fixed

symlinkPath := filepath.Join(os.TempDir(), hex.EncodeToString(randBytes))
if err := os.Symlink(targetPath, symlinkPath); err != nil {
return nil, err
}

result := &testSymlinkCreationResult{
symlink: symlinkPath,
}
if invalidTarget {
os.RemoveAll(targetPath)
}
result.target = targetPath

return result, nil
}
6 changes: 3 additions & 3 deletions go/pkg/tree/tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,14 @@ func loadFiles(execRoot string, excl []*command.InputExclusion, path string, fs
absPath := filepath.Clean(filepath.Join(execRoot, path))
meta := cache.Get(absPath)
t := command.FileInputType
if smd := meta.Symlink; smd != nil && smd.IsInvalid {
return nil
}
if meta.Err != nil {
e, ok := meta.Err.(*filemetadata.FileError)
if !ok {
return meta.Err
}
if e.IsInvalidSymlink {
return nil
}
if !e.IsDirectory {
return meta.Err
}
Expand Down