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

dep should prune testdata directories if go-tests pruner is enabled #1580

Closed
ixdy opened this issue Jan 25, 2018 · 10 comments
Closed

dep should prune testdata directories if go-tests pruner is enabled #1580

ixdy opened this issue Jan 25, 2018 · 10 comments

Comments

@ixdy
Copy link

ixdy commented Jan 25, 2018

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

v0.4.1

What dep command did you run?

I updated Gopkg.toml to vendor the dep command and to prune everything nonessential:

required = [
  "github.com/golang/dep/cmd/dep"
]

[[constraint]]
  name = "github.com/golang/dep"
  version = "v0.4.1"
  
[prune]
  unused-packages = true
  go-tests = true
  non-go = true

I then ran dep ensure.

What did you expect to see?

All non-used files and directories removed.

What did you see instead?

dep left the testdata directory in vendor/github.com/golang/dep/internal/fs, even though it's not used.

The infinite symlinks in this directory are causing problems for Bazel as described in #1412 (comment).

@sdboyer
Copy link
Member

sdboyer commented Jan 26, 2018

huh, interesting. i would've figured it removed those, unless an actual import points into them. /cc @ibrasho

@ixdy
Copy link
Author

ixdy commented Jan 26, 2018

In case you want to inspect, I made a branch with the change I described here: https://github.com/ixdy/kubernetes-repo-infra/tree/dep-testdata-dir

@sdboyer
Copy link
Member

sdboyer commented Jan 26, 2018

Ah yeah, i suspect our deletion logic is being tripped up by the circular symlinks. Yeah...really should get rid of those. hmm.

@ibrasho
Copy link
Collaborator

ibrasho commented Jan 26, 2018

@sdboyer Actually, this is not related to the circular symlinks. non-go doesn't delete symlinks by default. We only delete symlinks if they are named vendor. The only file deleted from internal/fs/testdata is internal/fs/testdata/test.file.

If we made non-go delete symlinks in unused packages it would have deleted the circular symlinks. But I'm pretty sure we had a reason to keep them, but I can't remember what it was... 😅

@ixdy
Copy link
Author

ixdy commented Jan 26, 2018

I guess I argued for the wrong pruner, originally. I suspect that having dep prune symlinks is likely to cause various edge cases to fail.

Instead, I would expect that setting go-tests = true would unilaterally remove any testdata directories encountered. This directory even has special meaning to go:

$ go help test
...
The go tool will ignore a directory named "testdata", making it available
to hold ancillary data needed by the tests.
...

It thus should be safe to remove it if all tests are also removed.

@ixdy ixdy changed the title dep should prune testdata directories dep should prune testdata directories if go-tests pruner is enabled Jan 26, 2018
@sdboyer
Copy link
Member

sdboyer commented Jan 27, 2018

@ibrasho ohh right. god, symlinks. we really need to keep better notes on these conversations we have about them 😢

Instead, I would expect that setting go-tests = true would unilaterally remove any testdata directories encountered. This directory even has special meaning to go:

that's an interesting point. now, it can't be entirely unconditional, but it seems like we should be able to incorporate something like that under the go-tests = true rule.

@ibrasho
Copy link
Collaborator

ibrasho commented Jan 27, 2018

@sdboyer I really should get to documenting how we handle symlinks in general...

If go treats this directory as a special case, similar to vendor, why don't we prune it along with go-tests?

@sdboyer
Copy link
Member

sdboyer commented Jul 24, 2018

(belated answer)

If go treats this directory as a special case, similar to vendor, why don't we prune it along with go-tests?

Because it's (annoyingly) allowed to have an import statement that points into a testdata dir. Same as with a _* or a .* dir.

Fortunately, once we stop just stop exporting symlinks, i think things will get a lot easier.

@BenTheElder
Copy link

BenTheElder commented Sep 14, 2018

Is this still planned? If not, for anyone else with this problem you can wrap dep in a thin script that rms them before and after running dep, which mostly works fine.
Edit: dep is otherwise generally a pleasure to work with these days, thanks for all the hard work :-)

@mvdan
Copy link
Member

mvdan commented Sep 4, 2020

Dep was officially deprecated earlier this year, and the proposal to archive this repository was accepted. As such, I'm closing outstanding issues before archiving the repository. For any further comments, please use the proposal thread on the Go issue tracker. Thanks!

@mvdan mvdan closed this as completed Sep 4, 2020
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 a pull request may close this issue.

5 participants