Skip to content
This repository has been archived by the owner on Feb 26, 2019. It is now read-only.

Fix sub dirs #415

Merged
merged 1 commit into from
Feb 22, 2016
Merged

Fix sub dirs #415

merged 1 commit into from
Feb 22, 2016

Conversation

freeformz
Copy link

TL;DR: We were vendoring too much, this should fix that w/o breaking
anything (tm).

While working with an internal project and a bunch of go1.6 repos I
observed go install ./... trying to compile stuff that is in the
vendor directory, but doesn't have all of the deps also vendored. This
means that they were being copied in incorrectly.

Upon inspection of the code I noticed that vcs files would list all
files and the sub directories and the dependency checks in fill would
skip packages based on import prefix, when (AFAICT) it should only skip
standard and the primary root package importPath.

I also discovered a few tests that appear to be faulty, or exhibit older
behaviour that I didn't even think existed.

Basically this commit makes it so that when filling godeps we include
all dependencies and when saving we don't automatically process sub
directories of packages unless those sub-directories are also packages
that are required by the dependency search.

This commit adjusts some tests which I hand validated for behavior.
Most tests though are unchanged. Also added positional identifier for
more save test errors.

More debug added for later operations of save/update.

TL;DR: We were vendoring too much, this should fix that w/o breaking
anything (tm).

While working with an internal project and a bunch of go1.6 repos I
observed `go install ./...` trying to compile stuff that is in the
vendor directory, but doesn't have all of the deps also vendored. This
means that they were being copied in incorrectly.

Upon inspection of the code I noticed that vcs files would list all
files and the sub directories and the dependency checks in fill would
skip packages based on import prefix, when (AFAICT) it should only skip
standard and the primary root package importPath.

I also discovered a few tests that appear to be faulty, or exhibit older
behaviour that I never reviewed.

Basically this commit makes it so that when filling godeps we include
all dependencies and when saving we don't automatically process sub
directories of packages unless those sub-directories are also packages
that are required by the dependency search.

This commit adjusts some tests which I hand validated for behaviour.
Most tests though are unchanged. Also added positional identifier for
more save test errors.

More debug added for later operations of save/update.
@cyx
Copy link

cyx commented Feb 20, 2016

I assume this hasn't been merged, correct?

@vrecan
Copy link

vrecan commented Feb 22, 2016

👍 I'm running into this issue as well!

@freeformz
Copy link
Author

Also note that this patch "increases" the number of listed dependencies because we're no longer doing the path prefix match in godep fill. Previously if you had something like:

package A/B, imports A/B/C
package A/B/C

You would get a dependency on A/B recorded and then everything (including any sub dirs) of A/B would be copied in, including A/B/C. So things would work, but the recorded deps would be off (in that they didn't include all transitive deps).

Now you get both dependencies recorded (A/B and A/B/C).

@freeformz
Copy link
Author

This was okay in the "workspace" days because of the rewrites or requirement to prefix go tool runs with godep. But with vendor/ you can just use the naked go tool and ./... really should just work when everything is vendored properly. This should (barring other issues) fix that after a re-save of dependencies.

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

Successfully merging this pull request may close these issues.

3 participants