Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

dep, internal/gps: Always return an error from Analyzer.DeriveManifestAndLock #595

Closed
wants to merge 1 commit into from
Closed

dep, internal/gps: Always return an error from Analyzer.DeriveManifestAndLock #595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 0 additions & 4 deletions analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ func (a Analyzer) DeriveManifestAndLock(path string, n gps.ProjectRoot) (gps.Man
// TODO: If we decide to support other tools manifest, this is where we would need
// to add that support.
mf := filepath.Join(path, ManifestName)
if fileOK, err := IsRegular(mf); err != nil || !fileOK {
// Do not return an error, when does not exist.
return nil, nil, nil
}
f, err := os.Open(mf)
if err != nil {
return nil, nil, err
Expand Down
10 changes: 7 additions & 3 deletions analyzer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package dep

import (
"os"
"path/filepath"
"testing"

Expand Down Expand Up @@ -55,9 +56,12 @@ func TestAnalyzerDeriveManifestAndLockDoesNotExist(t *testing.T) {

a := Analyzer{}

m, l, err := a.DeriveManifestAndLock(h.Path("dep"), "my/fake/project")
if m != nil || l != nil || err != nil {
t.Fatalf("expected manifest & lock & err to be nil: m -> %#v l -> %#v err-> %#v", m, l, err)
_, _, err := a.DeriveManifestAndLock(h.Path("dep"), "my/fake/project")
if err == nil {
t.Fatalf("expected non nil error for non existant project, got: %v", err)
}
if !os.IsNotExist(err) {
t.Fatalf("expected IsNotExist error for non existant project, got: %v", err)
}
}

Expand Down
4 changes: 4 additions & 0 deletions internal/gps/vcs_source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ func (bs *baseVCSSource) getManifestAndLock(ctx context.Context, pr ProjectRoot,
}

m, l, err := an.DeriveManifestAndLock(bs.repo.LocalPath(), pr)
if os.IsNotExist(err) {
return prepManifest(nil), nil, nil
Copy link
Member

Choose a reason for hiding this comment

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

This is essentially guaranteed to be unreachable by a slew of code above it - we shouldn't be able to make it this far if the dir doesn't exist. It should only be reachable under active sabotage (or, ofc, a bug) - e.g., some other process removing the dir within the source cache while a dep command is running.

Even under such circumstances, it's relatively unlikely this would be the spot that happens to first notice the dir is absent. If it is, though, it's preferable to error out, rather than pass up the "zero" result, as non-error results get cached. If/when that cache becomes persistent across runs (#431), that incorrect information could plague future runs.

So, better to just propagate the error right back out than try to recover.

Copy link

@dcheney-atlassian dcheney-atlassian May 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@sdboyer if I remove the special case for os.IsNotExist then a slew of tests fail in cmd/dep.

http://paste.ubuntu.com/24590545/

I wonder if something is messed up with the logic that lets this path get through to this point. Or maybe the test fixtures are broken?

}

if err != nil {
return nil, nil, err
}
Expand Down