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

[WIP] dep: Enable importing other tools and add integration tests #1277

Merged
merged 5 commits into from
Nov 10, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
5 changes: 4 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ BUG FIXES:

* Releases targeting Windows now have a `.exe` suffix (#1291).

WIP:

* dep: Enable importing other tools and add integration tests (#1277)
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 change this to Enable importing external configuration from dependencies during init, so that the scope of the change is more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a better comment, updated it.


IMPROVEMENTS:

* Log as dependencies are pre-fetched during dep init ([#1176](https://github.com/golang/dep/pull/1176)).
Expand Down Expand Up @@ -51,7 +55,6 @@ WIP:
* gps: Process canonical import paths. (#1017)
* gps: Persistent cache. (#1127, #1215)


# v0.3.1

* gps: Add satisfiability check for case variants (#1079)
Expand Down
3 changes: 1 addition & 2 deletions cmd/dep/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,7 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error {
}
}

rootAnalyzer.skipTools = true // Don't import external config during solve for now
copyLock := *p.Lock // Copy lock before solving. Use this to separate new lock projects from solved lock
copyLock := *p.Lock // Copy lock before solving. Use this to separate new lock projects from solved lock

params := gps.SolveParameters{
RootDir: root,
Expand Down
6 changes: 5 additions & 1 deletion cmd/dep/root_analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ func (a *rootAnalyzer) importManifestAndLock(dir string, pr gps.ProjectRoot, sup
if err != nil {
return nil, nil, err
}
a.removeTransitiveDependencies(m)
return m, l, err
}
}
Expand Down Expand Up @@ -165,6 +164,11 @@ func (a *rootAnalyzer) DeriveManifestAndLock(dir string, pr gps.ProjectRoot) (gp
}

func (a *rootAnalyzer) FinalizeRootManifestAndLock(m *dep.Manifest, l *dep.Lock, ol dep.Lock) {
// Transitive dependencies could sneak into the manifest when other importers are used
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is another open issue to appropriately handle imported constraints during solve: #1316. Would you please back out this change?

Since #1316 is going to impact many of your test cases, and you have a great set of data, if backing out this change breaks something, we can review and disable temporarily until #1316 is in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are looking to migrate folks over from glide to dep internally asap. I will remove it from this PR and we will use this code internally for now until #1316 is done. Please let me know if I can help in any way to expedite that issue. We can discuss how we want to tackle it and perhaps I can help with the changes?

I will add a comment of our need in #1316 , which is basically also needing to import transient dependencies of the root repo and add them to constraints to enable correct resolving.

if !a.skipTools {
a.removeTransitiveDependencies(m)
}

// Iterate through the new projects in solved lock and add them to manifest
// if they are direct deps and log feedback for all the new projects.
for _, y := range l.Projects() {
Expand Down
2 changes: 1 addition & 1 deletion cmd/dep/testdata/harness_tests/init/glide/case3/README.md
Original file line number Diff line number Diff line change
@@ -1 +1 @@
Import glide config in dependencies.
Import glide config in direct dependencies.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Import deptestglideA tag v0.3.0 which has a corrupt glide manifest.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package: github.com/golang/notexist
homepage: http://example.com
license: MIT
owners:
- name: Sam Boyer
email: sdboyer@example.com
homepage: http://sdboyer.io
import:
- package: github.com/ChinmayR/deptestglideA
version: v0.3.0
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright 2017 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package main

import (
"github.com/ChinmayR/deptestglideA"
)

type PointToDepTestGlideAv010 deptestglideA.Bversion2
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"commands": [
["init", "-no-examples", "-v"]
],
"error-expected": "v0.3.0: unable to parse"
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, I am going to keep this test case for now but once #1315 (which is also part of the epic) is implemented, it will probably be removed because the behavior is changing to not hard fail during solve. The testdata in your repo will be kept and used though, thank you for building that out! ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Take a direct dependency where the version is defined by glide.
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 remove this since the scenario is already covered by glide/case1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed that, will remove it.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@

[[constraint]]
name = "github.com/ChinmayR/deptestglideB"
version = "0.1.0"

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package: github.com/golang/notexist
homepage: http://example.com
license: MIT
owners:
- name: Sam Boyer
email: sdboyer@example.com
homepage: http://sdboyer.io
import:
- package: github.com/ChinmayR/deptestglideB
version: v0.1.0
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright 2017 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package main

import (
"github.com/ChinmayR/deptestglideB"
)

type FooVersion1 deptestglideB.FooVersion1
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"commands": [
["init", "-no-examples", "-v"]
],
"vendor-final": [
"github.com/ChinmayR/deptestglideB"
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Take a direct dependency on a transient dependency where the versions are conflicted. Resolving should fail.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this test starts failing after the other changes I have requested are made, let's ignore it test temporarily ( add a .ignore file extension to the testcase.json file). The scenario may require additional handling and will be addressed in another issue: #1316.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test does not fail since the existing code only removes transient dependencies read from importers. Since there is also a direct dependency here, the test passes.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package: github.com/golang/notexist
homepage: http://example.com
license: MIT
owners:
- name: Sam Boyer
email: sdboyer@example.com
homepage: http://sdboyer.io
import:
- package: github.com/ChinmayR/deptestglideA
version: v0.1.0
- package: github.com/ChinmayR/deptestglideB
version: v0.2.0
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright 2017 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package main

import (
"github.com/ChinmayR/deptestglideA"
"github.com/ChinmayR/deptestglideB"
)

type PointToDepTestGlideAv010 deptestglideA.Bversion1
type FooVersion2 deptestglideB.FooVersion2
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"commands": [
["init", "-no-examples", "-v"]
],
"error-expected": "master: Could not introduce github.com/ChinmayR/deptestglideA@master, as it is not allowed by constraint ^0.1.0 from project github.com/golang/notexist."
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Take a direct dependency on a transient dependency where the versions are not conflicted. Resolving should pass.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this test starts failing after the other changes I have requested are made, let's ignore it test temporarily ( add a .ignore file extension to the testcase.json file). The scenario may require additional handling and will be addressed in another issue: #1316.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test does not fail since the existing code only removes transient dependencies read from importers. Since there is also a direct dependency here, the test passes.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@

[[constraint]]
name = "github.com/ChinmayR/deptestglideA"
version = "0.2.0"

[[constraint]]
name = "github.com/ChinmayR/deptestglideB"

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package: github.com/golang/notexist
homepage: http://example.com
license: MIT
owners:
- name: Sam Boyer
email: sdboyer@example.com
homepage: http://sdboyer.io
import:
- package: github.com/ChinmayR/deptestglideA
version: v0.2.0
- package: github.com/ChinmayR/deptestglideB
version: ~0.1.0
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright 2017 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package main

import (
"github.com/ChinmayR/deptestglideA"
"github.com/ChinmayR/deptestglideB"
)

type PointToDepTestGlideAv010 deptestglideA.Bversion2
type FooVersion2 deptestglideB.FooVersion2
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"commands": [
["init", "-no-examples", "-v"]
],
"vendor-final": [
"github.com/ChinmayR/deptestglideA",
"github.com/ChinmayR/deptestglideB"
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Have two transient dependencies have different versions of the same repo. Resolving should fail.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is asserting an unwanted behavior that will be addressed in #1316. But it's a useful test case for that other story, so let's not remove it. Would you please add a .ignore to the testcase.json, and when we implement the other issue, we'll turn this test on, with the correct expected results (i.e. it should not fail).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test starts to fail since we do not import transient dependencies as constraints of the root repo. This results in incorrect resolving since only constraints that are imported are of direct dependencies of the root. I will add a comment to #1316 to also handle this case explicitly. Shouldn't the resolving for this test fail when the test case is enabled again as part of #1316 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe preferred versions handle this case but currently we have no idea of dealing with conflicting preferred versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'm a bit fuzzy off the top of my head on the topic of conflicting preferred versions. 😊 But that is a dep-wide question, not just for import, so I'd rather deal with it in that other issue and rope in Sam then.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package: github.com/golang/notexist
homepage: http://example.com
license: MIT
owners:
- name: Sam Boyer
email: sdboyer@example.com
homepage: http://sdboyer.io
import:
- package: github.com/ChinmayR/deptestglideA
version: v0.4.0
- package: github.com/ChinmayR/deptestglideB
version: v0.3.0
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright 2017 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package main

import (
"github.com/ChinmayR/deptestglideA"
"github.com/ChinmayR/deptestglideB"
)

type PointToDepTestGlideCv010 deptestglideA.Cversion1
type PointToDepTestGlideCv020 deptestglideB.Cversion2
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"commands": [
["init", "-no-examples", "-v"]
],
"error-expected": "No versions of github.com/ChinmayR/deptestglideB met constraints"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test if a transitive glide manifest is read.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@

[[constraint]]
name = "github.com/ChinmayR/deptestglideA"
version = "0.6.0"

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package: github.com/golang/notexist
homepage: http://example.com
license: MIT
owners:
- name: Sam Boyer
email: sdboyer@example.com
homepage: http://sdboyer.io
import:
- package: github.com/ChinmayR/deptestglideA
version: v0.6.0
Loading