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

Add default prune options for init cmd #1460

Merged
merged 2 commits into from
Dec 24, 2017
Merged

Add default prune options for init cmd #1460

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
701c109
Add default prune settings for init cmd
arbourd Dec 17, 2017
dd15e17
Add panic for non-empty ProjectOptions
arbourd Dec 22, 2017
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
3 changes: 3 additions & 0 deletions cmd/dep/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error {
return errors.Wrap(err, "init failed: unable to prepare an initial manifest and lock for the solver")
}

// Set default prune options for go-tests and unused-packages
p.Manifest.PruneOptions.PruneOptions = gps.PruneNestedVendorDirs + gps.PruneGoTestFiles + gps.PruneUnusedPackages

if cmd.gopath {
gs := newGopathScanner(ctx, directDeps, sm)
err = gs.InitializeRootManifestAndLock(p.Manifest, p.Lock)
Expand Down
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/add/all-new-double-spec/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
branch = "master"
name = "github.com/sdboyer/deptesttres"

[prune]
go-tests = true
unused-packages = true

[[constraint]]
name = "github.com/sdboyer/deptest"
version = "0.8.1"
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/add/all-new-double/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
name = "github.com/sdboyer/deptest"
version = "1.0.0"

[prune]
go-tests = true
unused-packages = true

[[constraint]]
branch = "master"
name = "github.com/sdboyer/deptesttres"
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/add/all-new-spec/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
branch = "master"
name = "github.com/sdboyer/deptesttres"

[prune]
go-tests = true
unused-packages = true

[[constraint]]
name = "github.com/sdboyer/deptest"
version = "0.8.1"
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/add/all-new/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
branch = "master"
name = "github.com/sdboyer/deptesttres"

[prune]
go-tests = true
unused-packages = true

[[constraint]]
name = "github.com/sdboyer/deptest"
version = "1.0.0"
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/add/errs/double-diff-spec/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
[[constraint]]
branch = "master"
name = "github.com/sdboyer/deptesttres"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/add/errs/self-add/case2/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/empty/case1/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
[[constraint]]
name = "github.com/sdboyer/deptest"
version = "1.0.0"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/pkg-errors/case1/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/case1/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
[[constraint]]
name = "github.com/sdboyer/deptest"
version = "0.8.0"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/case2/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@
[[constraint]]
name = "github.com/sdboyer/deptestdos"
version = "2.0.0"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/case3/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
[[constraint]]
branch = "master"
name = "github.com/sdboyer/deptest"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/case4/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@
[[constraint]]
name = "github.com/sdboyer/deptestdos"
version = "2.0.0"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/glide/case1/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,7 @@ ignored = ["github.com/golang/notexist/samples","github.com/sdboyer/dep-test"]
[[constraint]]
name = "github.com/sdboyer/deptestdos"
version = "2.0.0"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/glide/case2/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
[[constraint]]
name = "github.com/sdboyer/deptestdos"
version = "2.0.0"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/glide/case3/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
[[constraint]]
name = "github.com/carolynvs/deptestglide"
version = "0.1.1"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/glock/case1/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
[[constraint]]
name = "github.com/sdboyer/deptestdos"
version = "2.0.0"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/godep/case1/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
[[constraint]]
name = "github.com/sdboyer/deptestdos"
version = "2.0.0"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/govend/case1/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
[[constraint]]
name = "github.com/sdboyer/deptestdos"
version = "2.0.0"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/govendor/case1/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,7 @@ ignored = ["github.com/golang/notexist/samples*","github.com/sdboyer/dep-test*"]
[[constraint]]
name = "github.com/sdboyer/deptestdos"
version = "2.0.0"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/gvt/case1/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,7 @@
[[constraint]]
branch = "v2"
name = "gopkg.in/yaml.v2"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/relative_path/final/project_dir/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
[[constraint]]
branch = "master"
name = "github.com/sdboyer/deptest"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/skip-hidden/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
[[constraint]]
name = "github.com/sdboyer/deptest"
version = "1.0.0"

[prune]
go-tests = true
unused-packages = true
4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/vndr/case1/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
[[constraint]]
name = "github.com/sdboyer/deptestdos"
version = "2.0.0"

[prune]
go-tests = true
unused-packages = true
28 changes: 22 additions & 6 deletions manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,23 @@ func fromRawPruneOptions(raw rawPruneOptions) gps.RootPruneOptions {
return opts
}

func toRawPruneOptions(options gps.PruneOptions) rawPruneOptions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a unit test. I realize we have integration tests that are tangentially testing this, but they don't exercise all of the flags.

Copy link
Collaborator

Choose a reason for hiding this comment

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

After this is updated to reflect the changes in #1219 , how can we clarify that this only works during the initial init? Unmarshalling project prune options is not possible without too much hacking...

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ibrasho I am not sure I understand the question/problem?

Copy link
Collaborator

@ibrasho ibrasho Dec 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Apologies for not explaining thoroughly. 😅

Do you remember the issue with unmarshalling prune project options [1] [2] (since we have a mixed string/bools TOML table)?

In short, this function (while it's only used in init) can be used later and assumed to write the parsed manifest as-read. While in reality, it would at best ignore prune project options.

This might not be an issue now, but I want us to make sure that we don't forget this later and assume that this function can un-marshall the full prune options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just an observation: this function is also called during ensure -add.

Copy link
Member

Choose a reason for hiding this comment

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

if we have a way of encoding that failure mode as a panic, that would be great. I'm not sure offhand if it's possible, but something like "check if the map of project-specific options is non-empty,and if so, panic"

Copy link
Member

Choose a reason for hiding this comment

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

yeah...the call during the new mode is troubling. really, that should only be operating on discrete, additional items, not a whole Manifest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ibrasho Let me repeat back what I am hearing to make sure I've got it.

The concern is that this function only writes global prune options. If later someone tried to read in the Gopkg.toml, modify the in-memory manifest, and then write back the entire manifest to disk that the project level prune options would be lost?

I read through the code for dep ensure --add and have seen the manifest output from this PR. It doesn't appear to be behaving incorrectly. So I'd like to set that aside for now.

If you agree that I've finally understood your concern(?), then my initial preference would have been to fully implement the conversion from a manifest to toml. Unfortunately, it appears from your comment that the conversion of project-level prune options to a raw representation is not easy and would require hacks. I'll take your word on that! 😀

Since the full implementation is not actually needed, Sam's suggestion is to raise a panic when project-level prune options are present, forcing the developer to deal with this in the future if they try to use the unimplemented functionality.

Let me know if I've summarized correctly. If I have, then yes, I would also prefer a panic (and perhaps a copy/paste of my summary or a link back to it), over just a comment.

Copy link
Collaborator

@ibrasho ibrasho Dec 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

A panic is a better and clearer approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you change it to accept a gps.RootPruneOptions, so we can check if options.ProjectOptions is not empty?

raw := rawPruneOptions{}

if (options & gps.PruneUnusedPackages) != 0 {
raw.UnusedPackages = true
}

if (options & gps.PruneNonGoFiles) != 0 {
raw.NonGoFiles = true
}

if (options & gps.PruneGoTestFiles) != 0 {
raw.GoTests = true
}
return raw
}

// toProject interprets the string representations of project information held in
// a rawProject, converting them into a proper gps.ProjectProperties. An
// error is returned if the rawProject contains some invalid combination -
Expand Down Expand Up @@ -462,11 +479,10 @@ func (m *Manifest) MarshalTOML() ([]byte, error) {
// toRaw converts the manifest into a representation suitable to write to the manifest file
func (m *Manifest) toRaw() rawManifest {
raw := rawManifest{
Constraints: make([]rawProject, 0, len(m.Constraints)),
Overrides: make([]rawProject, 0, len(m.Ovr)),
Ignored: m.Ignored,
Required: m.Required,
PruneOptions: rawPruneOptions{},
Constraints: make([]rawProject, 0, len(m.Constraints)),
Overrides: make([]rawProject, 0, len(m.Ovr)),
Ignored: m.Ignored,
Required: m.Required,
}

for n, prj := range m.Constraints {
Expand All @@ -479,7 +495,7 @@ func (m *Manifest) toRaw() rawManifest {
}
sort.Sort(sortedRawProjects(raw.Overrides))

// TODO(ibrasho): write out prune options.
raw.PruneOptions = toRawPruneOptions(m.PruneOptions.PruneOptions)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend passing m.PruneOptions instead. So we can check if the projects are empty or not in toRawPruneOptions.


return raw
}
Expand Down
37 changes: 37 additions & 0 deletions manifest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,43 @@ func TestValidateProjectRoots(t *testing.T) {
}
}

func TestToRawPruneOptions(t *testing.T) {
cases := []struct {
name string
pruneOptions gps.PruneOptions
wantOptions rawPruneOptions
}{
{
name: "all options",
pruneOptions: 15,
wantOptions: rawPruneOptions{
UnusedPackages: true,
NonGoFiles: true,
GoTests: true,
},
},
{
name: "no options",
pruneOptions: 1,
wantOptions: rawPruneOptions{
UnusedPackages: false,
NonGoFiles: false,
GoTests: false,
},
},
}

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
raw := toRawPruneOptions(c.pruneOptions)

if !reflect.DeepEqual(raw, c.wantOptions) {
t.Fatalf("rawPruneOptions are not as expected:\n\t(GOT) %v\n\t(WNT) %v", raw, c.wantOptions)
}
})
}
}

func containsErr(s []error, e error) bool {
for _, a := range s {
if a.Error() == e.Error() {
Expand Down
9 changes: 7 additions & 2 deletions testdata/txn_writer/expected_manifest.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,13 @@
# source = "github.com/myfork/project2"
#
# [[override]]
# name = "github.com/x/y"
# version = "2.4.0"
# name = "github.com/x/y"
# version = "2.4.0"
#
# [prune]
# non-go = false
# go-tests = true
# unused-packages = true


[[constraint]]
Expand Down
9 changes: 7 additions & 2 deletions txn_writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,13 @@ var exampleTOML = []byte(`# Gopkg.toml example
# source = "github.com/myfork/project2"
#
# [[override]]
# name = "github.com/x/y"
# version = "2.4.0"
# name = "github.com/x/y"
# version = "2.4.0"
#
# [prune]
# non-go = false
# go-tests = true
# unused-packages = true
`)

Expand Down