search

golang / dep

Go dependency management tool experiment (deprecated)
https://golang.github.io/dep/
BSD 3-Clause "New" or "Revised" License
12.85k stars 1.05k forks source link

Prune should remove contents of root package directory when only subpackages are used #1113

Closed kevinburke closed 6 years ago

kevinburke commented 7 years ago

What version of dep are you using (dep version)?

v0.1.0-585-g34835a5

What dep command did you run?

dep init; dep ensure; dep prune.

I've set up a test repository with the results here: https://github.com/kevinburke/dep-test-bson

What did you expect to see?

I expected the vendor directory to have the following contents (and only the following contents)

vendor/gopkg.in/mgo.v2/bson/ vendor/gopkg.in/mgo.v2/internal/json/

What did you see instead?

I see all of the files and contents in the gopkg.in/mgo.v2 package as well:

$ ll vendor/gopkg.in/mgo.v2
total 1104
drwxr-xr-x  33 kevin  staff   1.1K Sep  1 13:27 .
drwxr-xr-x   3 kevin  staff   102B Sep  1 13:27 ..
-rw-r--r--   1 kevin  staff   1.0K Sep  1 13:27 .travis.yml
-rw-r--r--   1 kevin  staff   1.3K Sep  1 13:27 LICENSE
-rw-r--r--   1 kevin  staff    67B Sep  1 13:27 Makefile
-rw-r--r--   1 kevin  staff   136B Sep  1 13:27 README.md
-rw-r--r--   1 kevin  staff    12K Sep  1 13:27 auth.go
-rw-r--r--   1 kevin  staff    31K Sep  1 13:27 auth_test.go
drwxr-xr-x  12 kevin  staff   408B Sep  1 13:27 bson
-rw-r--r--   1 kevin  staff   9.8K Sep  1 13:27 bulk.go
-rw-r--r--   1 kevin  staff    14K Sep  1 13:27 bulk_test.go
-rw-r--r--   1 kevin  staff    19K Sep  1 13:27 cluster.go
-rw-r--r--   1 kevin  staff    54K Sep  1 13:27 cluster_test.go
-rw-r--r--   1 kevin  staff   1.2K Sep  1 13:27 doc.go
-rw-r--r--   1 kevin  staff   586B Sep  1 13:27 export_test.go
-rw-r--r--   1 kevin  staff    21K Sep  1 13:27 gridfs.go
-rw-r--r--   1 kevin  staff    16K Sep  1 13:27 gridfs_test.go
drwxr-xr-x   3 kevin  staff   102B Sep  1 13:27 internal
-rw-r--r--   1 kevin  staff   3.8K Sep  1 13:27 log.go
-rw-r--r--   1 kevin  staff   2.8K Sep  1 13:27 queue.go
-rw-r--r--   1 kevin  staff   2.7K Sep  1 13:27 queue_test.go
-rw-r--r--   1 kevin  staff    57B Sep  1 13:27 raceoff.go
-rw-r--r--   1 kevin  staff    55B Sep  1 13:27 raceon.go
-rw-r--r--   1 kevin  staff   224B Sep  1 13:27 saslimpl.go
-rw-r--r--   1 kevin  staff   194B Sep  1 13:27 saslstub.go
-rw-r--r--   1 kevin  staff    12K Sep  1 13:27 server.go
-rw-r--r--   1 kevin  staff   145K Sep  1 13:27 session.go
-rw-r--r--   1 kevin  staff   103K Sep  1 13:27 session_test.go
-rw-r--r--   1 kevin  staff    18K Sep  1 13:27 socket.go
-rw-r--r--   1 kevin  staff   3.4K Sep  1 13:27 stats.go
-rw-r--r--   1 kevin  staff   5.9K Sep  1 13:27 suite_test.go
-rw-r--r--   1 kevin  staff   218B Sep  1 13:27 syscall_test.go
-rw-r--r--   1 kevin  staff   181B Sep  1 13:27 syscall_windows_test.go
kevinburke commented 7 years ago

Previously, previously, previously

carolynvs commented 7 years ago

@kevinburke Am I summarizing correctly that you were expecting the root package contents (e.g. gopkg.in/mgo.v2/stats.go) to be pruned because your code is only importing a subpackage?

sdboyer commented 7 years ago

ohh interesting - yeah prune does need to empty out leading dirs that have to be there for structural purposes/because subdirs are required.

kevinburke commented 7 years ago

Carolyn - yes, that's correct

On Fri, Sep 1, 2017 at 13:41 sam boyer notifications@github.com wrote:

ohh interesting - yeah prune does need to empty out leading dirs that have to be there for structural purposes/because subdirs are required.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/golang/dep/issues/1113#issuecomment-326679165, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOSI-aTv4LykxZh0Men1_mp9LclG0z_ks5seGvbgaJpZM4PKmZ4 .

--

-- Kevin Burke 925.271.7005 | kev.inburke.com

ibrasho commented 7 years ago

1020 does this in the pruneUnusedPackages func.

carolynvs commented 7 years ago

@ibrasho Is there a test in that PR that demonstrates unused packages being pruned?

ibrasho commented 7 years ago

Not yet. It was in the original PR. I'm still working on completing #1020.

ibrasho commented 7 years ago

Since this is a separate issue... :grin:

Which files should be deleted in an "usused" package? Currently, #1020 deletes all files (including symlinks) except those matching some patterns (legal files and such). Is this the correct behavior?

Pruning options will be definable per-project, so this should pose a small issue anyway.

ibrasho commented 7 years ago

@sdboyer I'm not sure about deleting symlinks tbh. If a package relies on symlinks for package name this could cause unexpected issues.

kevinburke commented 6 years ago

This also causes a poor interaction with Bazel. For example, I only need golang.org/x/crypto/ssh/terminal. But because golang.org/x/crypto/ssh has all of the Go files in it, Bazel generates a BUILD.bazel file for it, which then means that Bazel expects all of the dependencies for golang.org/x/crypto/ssh to be in my vendor directory, even though they're unnecessary.

More discussion here: https://github.com/bazelbuild/rules_go/issues/725

MarkusTeufelberger commented 6 years ago

I would expect the following behavior for dep ensure:

After running this command, all files in vendor/... are there because they either have to be there because of

Everything else (go code or not) that is in the vendor folder, but does NOT influence the compilation of the actual project and is not there for the first 2 reasons is useless and must be removed from the vendor folder.

This would solve issues deciding what should be kept: optional dependencies (library A depends on library B in some cases, but the actual project doesn't use these parts of library A anyways), symlinks (if they are not present and the project still compiles the same way, they are not necessary), random other files (.travis.yml really doesn't need to be vendored...) and so on could be evaluated against these criteria, even automatically by looking at hashes of compilation units.

sdboyer commented 6 years ago

oh, this is fixed now.

@MarkusTeufelberger pruning behavior is now established. fixes are possible, of course, but they need to be in a new issue. (and I'm not actually sure if you're trying to propose a change to the v0.4.1 behavior here)

MarkusTeufelberger commented 6 years ago

I'm not actually sure if you're trying to propose a change to the v0.4.1 behavior here

Neither am I, is it documented somewhere in detail?

https://github.com/golang/dep/issues/1580 for example sounds like it is not working as I would expect it to work (vendored testdata or vendored tests in general shouldn't be able to influence the outcome of a compilation).

"unused-packages indicates that files from directories that do not appear in the package import graph should be pruned." from https://golang.github.io/dep/docs/Gopkg.toml.html#prune sounds close, I guess I'm proposing an unused-files mode:

"unused-files indicates that all files that do not cause a change in the resulting package should be pruned."

I can of course open an issue, if that sounds like a reasonable proposal.

sdboyer commented 6 years ago

you linked to the docs we have so far. writing up a section to further explore pruning is a TODO.

that specific issue is a bit of an oddity, and is more of a bug than anything. however, it is not the case that we can necessarily assume testdata can be discarded - it is a terrible idea, but not disallowed to import from non-test Go code (and I've seen it in the wild in some protobuf library somewhere). so, we can't really treat the contents of those directories specially with respect to pruning rules.

how is what you're describing different from non-go?

(actually, yes, please open a new issue for this, and answer that question there 😊)

MarkusTeufelberger commented 6 years ago

See #1614