WIP - Import godep #589

darkowlzz · 2017-05-16T22:30:39Z

Refer #577

Built on top of #500

Adds godep_config.go, godep_importer.go and tests.

Extract initial root manifest generation into an analyzer which combines evaluating the contents of the GOPATH with importing cfg from other managers.

This makes it easier for the importers to reuse its logic

* Warn that os, arch or subpackages specified in glide.yaml are ignored * Require the package.name field

googlebot · 2017-05-16T22:30:48Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

carolynvs · 2017-05-16T23:37:03Z

@darkowlzz Woooooo! Reviewing now, I'll do my best to not cause too much grief as I work on getting #500 merged.

carolynvs

FYI, there will be more changes most likely incoming as a result of whatever changes are requested against #500.

carolynvs · 2017-05-17T02:48:13Z

cmd/dep/godep_config.go

+	c, has := manifest.Dependencies[id.ProjectRoot]
+	if has {
+		// Create PairedVersion if constraint details are available
+		version := gps.NewVersion(c.Constraint.String())


Doh! I completely forgot to pair the version in my PR. Nice catch! 😊

carolynvs · 2017-05-17T03:14:40Z

cmd/dep/godep_config_test.go

+		t.Fatalf("Expected the lock to have a project for 'github.com/sdboyer/deptest' but got '%s'", p.Ident().ProjectRoot)
+	}
+
+	lv := p.Version().String()


Let's add a check that the version was paired with a revision. After I updated glide to copy what you are doing, I updated the test with this:

https://github.com/carolynvs/dep/blob/import-glide/cmd/dep/glide_config_test.go#L84

carolynvs · 2017-05-17T13:28:57Z

cmd/dep/testdata/harness_tests/init/godep/case2/testcase.json

+    ["init", "-no-examples", "-skip-tools"]
+  ],
+  "error-expected": "",
+  "gopath-initial": {},


Let's pre-populate the gopath with the dependencies. This will:

a. Give us control over the versions used, so that they don't collide with the same versions in case 1. Right now the final Gopkg.lock is exactly the same between the two cases, making it hard to tell that the -skip-tools flag worked.
b. Protect the test from failing when new releases are added to the test repositories. By omitting the repo from gopath, the test now depends on pulling the repo from the network.

- Adds test for PariedVersion in godep_config. - Adds dependency constraints in gopath-initial for godep integration tests when -skip-tools is used. This differentiates final locks for with and without tools.

darkowlzz · 2017-05-17T19:03:39Z

Added the suggested test changes.

FYI, there will be more changes most likely incoming as a result of whatever changes are requested against #500.

@carolynvs that's alright. Now we have working godep importer code. Once #500 is merged, we can just take the few godep parts from this PR and use them later.
Thanks for reviewing :)

sdboyer · 2017-05-17T19:11:37Z

without having looked at the code in the PR yet at all, lemme just note that, given that Godep only ever stores commit hashes, probably the best route to follow is to transform it only into a lock - no manifest. We should also attempt to convert commit hashes into more friendly user-facing versions, if any are available.

darkowlzz · 2017-05-18T00:12:34Z

cmd/dep/composite_analyzer.go

+							rootL.P[i] = lp
+						}
+					}
+				}


@carolynvs a little problem here. Overlaying of lock isn't happening in some cases.

Let's take the example of integration test init/godep/case1/.
In godep/case1/testcase.json, although gopath-initial contains deptest, since deptest is a transitive dependency, the default (first analyzer) doesn't adds it to the manifest or lock it generates. And since there's no deptestdos in gopath-initial, the first analyzer generates empty (not nil) manifest and lock structs.
Now in the above code, when overlay is attempted for lock with godep analyzer generated manifest and lock,

for i, existingLP := range rootL.P {

is skipped because rootL is empty from the first analyzer run. And no lock overlay happens.

Makes sense?

The final lock in init/godep/case1/ is incorrect at the moment. Commit hash in final lock is different from the one in Godep.json.

UPDATE: I see you have changed the overlay code in #500 and now it's handling this case :)

Yeah found that I broke overlay at some point. I have a bunch of fixes/pr feedback I'll push tonight. 😊

darkowlzz · 2017-05-18T21:01:17Z

cmd/dep/godep_config.go

+					break
+				}
+			}
+		}


@sdboyer @carolynvs ^ tries to fetch version for a given commit hash. Is there anything, that you can think of, is available and I could have reused?

That's how I would have done it. It seems useful enough that either a) it already exists in gps somewhere or b) would be a very nice function to extract out for other importers to use.

I had to do something similar for finding the default branch and ended up putting that logic into deduceConstraint. https://github.com/carolynvs/dep/blob/import-glide/cmd/dep/ensure.go#L310

There is - have a look at version_unifier.go. Actually, there's a whole pile of logic for this, but it's all hidden away unexported inside gps, as it's encoded as an extension to the Version system, and I didn't want to leak an extra type out there.

I'd strongly prefer we not leak that unifier type publicly - note the unconditional panics in certain of the interface's method implementations - but we could expose some alternate frontend to it.

I've also been thinking that we might want to export the source interface (and from there, sourceGateways), and include such methods on it. That's probably out of scope for immediate needs, though...

- Fetches versions list of project and looks for version corresponding to the given revision. - Adds test TestGodepConvertBadInput_EmptyPackageName for the same.

carolynvs · 2017-06-02T16:05:26Z

@darkowlzz #500 was just merged with lots of changes. Notably the config and importer structs were merged into a single struct and the interface tweaked slightly. Also I stole your logic for pairing a revision with a version and called the function lookupVersionForRevision. 💖

I know a ton has changed out from underneath you, happy to answer any questions or help move this over!

darkowlzz · 2017-06-10T20:32:42Z

Closing this in favor of #742 .

carolynvs added 14 commits May 15, 2017 09:34

Vendor go-yaml

4555892

Make it easier to tell when dep has found its own config

d4e3df4

Read glide config files

1b526a5

Read other dependency mgrs cfg during init

e3dd721

Extract initial root manifest generation into an analyzer which combines evaluating the contents of the GOPATH with importing cfg from other managers.

Test init -skip-tools ignores glide cfg

f2b54e1

Remove unused manifest deps

6941a1c

Add dep ensure package@version tests

54d535b

Consolidate version/branch validation into deduceConstraint

318ce20

This makes it easier for the importers to reuse its logic

Internally export NewDefaultBranch

14188a3

Categorize glide versions as a branch or version

b4380e3

Use the default branch when glide.yaml doesn’t specify

adc74e5

Tidy up new import interfaces

b1c99a8

Validate glide configuration file during import

c0deda7

* Warn that os, arch or subpackages specified in glide.yaml are ignored * Require the package.name field

Rename files to snake case

aa4a4c9

googlebot added the cla: no label May 16, 2017

darkowlzz mentioned this pull request May 16, 2017

Support importing godep config during dep init #577

Closed

darkowlzz added 3 commits May 17, 2017 04:26

Add godepFile load, convert and godep importer

393e718

Add godep importer integration tests

d2f3fe2

Add license headers

7398d50

darkowlzz force-pushed the import-godep branch from 377095e to 7398d50 Compare May 16, 2017 22:56

Check json.Unmarshal for error

5c59560

carolynvs suggested changes May 17, 2017

View reviewed changes

Improve godep import tests

c637683

- Adds test for PariedVersion in godep_config. - Adds dependency constraints in gopath-initial for godep integration tests when -skip-tools is used. This differentiates final locks for with and without tools.

darkowlzz commented May 18, 2017

View reviewed changes

Attempt to fetch version for no comment imports

f2b9cba

- Fetches versions list of project and looks for version corresponding to the given revision. - Adds test TestGodepConvertBadInput_EmptyPackageName for the same.

darkowlzz force-pushed the import-godep branch from b00a7c3 to f2b9cba Compare May 18, 2017 21:20

carolynvs mentioned this pull request May 23, 2017

Extract command implementions into sub-packages #609

Closed

carolynvs mentioned this pull request Jun 6, 2017

Alternate init modes #186

Closed

darkowlzz closed this Jun 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP - Import godep #589

WIP - Import godep #589

darkowlzz commented May 16, 2017

googlebot commented May 16, 2017

carolynvs commented May 16, 2017

carolynvs left a comment

carolynvs May 17, 2017

carolynvs May 17, 2017

carolynvs May 17, 2017

darkowlzz commented May 17, 2017

sdboyer commented May 17, 2017

darkowlzz May 18, 2017 •

edited

Loading

carolynvs May 18, 2017

darkowlzz May 18, 2017 •

edited

Loading

carolynvs May 19, 2017 •

edited

Loading

sdboyer May 21, 2017

carolynvs commented Jun 2, 2017

darkowlzz commented Jun 10, 2017

WIP - Import godep #589

WIP - Import godep #589

Conversation

darkowlzz commented May 16, 2017

googlebot commented May 16, 2017

carolynvs commented May 16, 2017

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs May 17, 2017

Choose a reason for hiding this comment

carolynvs May 17, 2017

Choose a reason for hiding this comment

carolynvs May 17, 2017

Choose a reason for hiding this comment

darkowlzz commented May 17, 2017

sdboyer commented May 17, 2017

darkowlzz May 18, 2017 • edited Loading

Choose a reason for hiding this comment

carolynvs May 18, 2017

Choose a reason for hiding this comment

darkowlzz May 18, 2017 • edited Loading

Choose a reason for hiding this comment

carolynvs May 19, 2017 • edited Loading

Choose a reason for hiding this comment

sdboyer May 21, 2017

Choose a reason for hiding this comment

carolynvs commented Jun 2, 2017

darkowlzz commented Jun 10, 2017

darkowlzz May 18, 2017 •

edited

Loading

darkowlzz May 18, 2017 •

edited

Loading

carolynvs May 19, 2017 •

edited

Loading