Skip to content
This repository has been 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

Conversation

davecheney
Copy link
Contributor

Analyzer.DeriveManifestAndLock returns a nil error when passed a non existent path. This is rather unidiomatic.

Instead, return the error to gps.getManifestAndLock and adjust it to handle this case explicitly.

…tAndLock

Analyzer.DeriveManifestAndLock returned a nil error when passed a non
existant file path. This is rather unidiomatic.

Instead, return the error to gps.getManifestAndLock and adjust it to handle
this case explicity.
@carolynvs
Copy link
Collaborator

The dep analyzer wasn't passed a non-existent path though. It was given the path to a dependency, and the analyzer checks if it had dep configuration that should be taken into account when solving.

@dcheney-atlassian
Copy link

@carolynvs IsRegular returns false, nil when the file is not found, DeriveManifestAndLock handles that case by returning nil, nil, nil, rather than propogating that error up to the caller.

@@ -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

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?

@sdboyer sdboyer dismissed their stale review May 17, 2017 03:08

misunderstood change

@sdboyer
Copy link
Member

sdboyer commented May 17, 2017

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?

OH. sorry, i misread the change - I thought this was dealing with the case where the parent dir doesn't exist - not where Gopkg.toml doesn't exist.

The logic here is correct as-is. The purpose of DeriveManifestAndLock() is to find manifest & lock metadata, if any exists. It's perfectly valid for there not to be any - that's the case for any [versions in a] repo that aren't using dep.

@sdboyer sdboyer closed this May 17, 2017
@davecheney
Copy link
Contributor Author

@sdboyer yup, this change does not change the logic here. It just moves the special handling of IsNotExist up to the caller so gps/Analyzer.DeriveManifestAndLock returns an error rather than relying on a convention of returning nil, nil, nil.

@sdboyer
Copy link
Member

sdboyer commented May 17, 2017

It doesn't change the outcome, but it violates the intention of the interface. It's not an error condition of DeriveManifestAndLock() to not find any of the files it may want to look for, and thus it should not return an error.

gps' internals have no idea how the manifest and lock data are being loaded by the analyzer; this change would remove that abstraction. It's probably reading files from disk, but maybe it's making calls into a database. Maybe it's reading multiple files on disk. Maybe it's searching for files from multiple different tools. Point is, gps doesn't know or care; if it gets back an error from DeriveManifestAndLock(), the analyzer is saying that version is unusable.

Now, I can certainly see this needing to be a docs improvement, as none of this is written out on the ProjectAnalyzer.DeriveManifestAndLock() docs 😄

@davecheney
Copy link
Contributor Author

davecheney commented May 17, 2017 via email

@davecheney
Copy link
Contributor Author

davecheney commented May 17, 2017 via email

@sdboyer
Copy link
Member

sdboyer commented May 17, 2017

No problem - if there's a way of expressing these same semantics that's more idiomatic, I'd be happy to change it (and would welcome the learning opportunity). Just so long as the separation of concerns is maintained 😄

sdboyer added a commit that referenced this pull request May 17, 2017
@davecheney
Copy link
Contributor Author

davecheney commented May 17, 2017 via email

@sdboyer
Copy link
Member

sdboyer commented May 17, 2017

gps didn't ask it to open Gopkg.toml, though - that's dep's implementation detail. It only provided the parent directory in which such a file might live. And that directory is present.

@dcheney-atlassian
Copy link

dcheney-atlassian commented May 17, 2017 via email

@davecheney
Copy link
Contributor Author

Right, but if you call a method that returns an error value then the rule
is; if error is not nil, then none of the other values are valid -- they
have no value, not even nil, you cannot touch them.

What I meant to say, but hit send to quickly was, in the opposite case where error is nil, the other values are usually assumed to be value -- ie, not nil. But this is not the case here.

@sdboyer
Copy link
Member

sdboyer commented May 17, 2017

This leads to the situation where the implementation of DeriveManifestAndLock
create https://github.com/golang/dep/blob/master/cmd/dep/init.go#L164 and
https://github.com/golang/dep/blob/master/cmd/dep/status.go#L257 return the
unusual nil, nil, nil when passed an invalid path.

Ah, this is an interesting point. Though I think it actually works against what you're going for - if an invalid path is passed - so, the parent directory, not one of the files - then DeriveManifestAndLock() should return an os.ErrNotExist. (This is the case I originally thought we were discussing). Doing so would create an ambiguity with the case you're focused on - is it the parent dir that doesn't exist, or the Gopkg.toml?

Also, while we don't support it now - it's also valid to return only lock data, but no manifest. (If we were to do on-the-fly conversion from Godep.json, that's how we'd likely do it). Clearly we don't want to return an error there...

This logic is componded by
https://github.com/golang/dep/blob/master/internal/gps/manifest.go#L148
which returns a SimpleManifest if prepManifest is passed a nil manifest,
which it will be in the case that the path passed to DeriveManifestAndLock
was not valid.

Again, the important question here is what "validity" means. The only way the input is invalid is if it's a path that doesn't exist/isn't a dir/etc.

We're basically discussing what the meaning of a nonexistent gps.Manifest is, in this context. If the goal is to avoid having the return nil, nil, nil case be meaningful, I can see two things we can do:

  1. Understanding the problems with it, if we wanted to create a different sentinel error to indicate this "no metadata found" scenario, which we then sniff for in gps, that'd do it.
  2. We could also specify that return nil, nil, nil is an invalid return combination, and instead expect the implementor to do what we already do - return gps.SimpleManifest{}, nil, nil to achieve the current effect.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants