New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alternate init modes #186

Closed
sdboyer opened this Issue Jan 28, 2017 · 26 comments

Comments

Projects
None yet
9 participants
@sdboyer
Copy link
Member

sdboyer commented Jan 28, 2017

To accommodate the variety of ways people currently manage their Go code and make the transition to dep as easy as possible, we want to give dep init different operational modes. This is a meta-issue for discussing and tracking all these different modes we might implement.

Currently, dep init works by:

  1. Analyzing the imports in your project
  2. Searching the GOPATH in which your project is located for these imports
  3. If not found, it adds the project to a list for gps to solve
  4. If found, it reads the vcs version from disk and uses that as the initial constraint in the manifest
  5. Step 4 is repeated transitively until we've run out of imports to visit
  6. If any imports weren't found on disk, we run a solve to pick versions for those, populating the manifest with versions from the solution (only for missing direct deps, though); they are added to the lock, but omitted from the manifest.

This is a reasonable model for people who've been using GOPATH in the conventional/vanilla way: it's basically just pulling up information from what's on disk into your manifest, so that dep can then replicate everything under vendor. However, this hardly the only way that may make sense for init to work.

Converting existing tools

First, there's converting over from an existing community tool. The basic approach here is to write a converter - something that reads the metadata files from the tool and translates them into the dep equivalent.

There's a bunch of prior art to work from here, fortunately: glide does these sorts of conversions automatically. In the work I was doing to slot gps in as the engine for glide, I updated the converters for gb, godep, gom, and gpm to be gps-style. These write to glide's version of manifests and locks, but those are quite analogous to dep, and should be easy to adapt. There's also an open PR for converting from govendor, though that's to existing glide structs, not the gps-adapted versions of them. (No help for gvt or anything else, unfortunately)

Now, we might ultimately decide we want to keep these tool-specific converters external to dep - maybe in a fully separate converter tool, or just ask the maintainers to add such commands themselves. Either way, though, it doesn't hurt to write them here now.

Just the network, please

Another alternate mode might be to ignore all the existing local information - GOPATH, existing tools, everything - and just have the solver pick out the latest versions it can. This might be surprisingly worthwhile, especially for projects that may have been lagging behind.

Interactive selection

All of these preceding options are more or less separate modes (with "just the network" being a supplemental fallback that can automatically fill in holes left by any others). However, there's another orthogonal option that we may want: prompting the user to select the version they want. I'm imagining two, mutually exclusive options:

  1. -prompt-missing, where the tool prompts the user to pick a version available upstream only if the main mode provides no useful constraint
  2. -prompt-all, where the user is prompted to choose from among the available upstream versions, and the version determined by the main inferencing mode (if any).

@sdboyer sdboyer added the help wanted label Jan 28, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jan 28, 2017

Let's do feedback and discussion on the general question of init modes here, but open up spinoff issues/PRs for each of the discrete parts.

@mattn

This comment has been minimized.

Copy link
Member

mattn commented Jan 28, 2017

I can write the converter of gom (my self) if you want.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jan 28, 2017

@mattn sounds great to me :) Open a new PR whenever you're ready, and just link it to this? I'll also probably create a label at some point.

@flibustenet

This comment has been minimized.

Copy link

flibustenet commented Jan 28, 2017

Why init doesn't look in vendor folder before GOPATH (like the app) ?

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jan 28, 2017

@flibustenet init relies on vcs metadata to determine versions. That metadata is usually present on GOPATH, and usually NOT present in vendor (most tools don't preserve it). Inferring versions without having vcs metadata is absurdly difficult in the general case.

More generally, though, init considers vendor to be a volatile - a place where code can be dumped. It does not represent user intent, per this graphic: #119 (comment) . At most, dep's interest in vendor is populating it, or verifying (#121) that it is what it should be.

@flibustenet

This comment has been minimized.

Copy link

flibustenet commented Jan 29, 2017

@sdboyer Thanks for your explanation, it is obvious.

@jbrodriguez

This comment has been minimized.

Copy link

jbrodriguez commented Jan 31, 2017

Just the network, please 👍

@nathany

This comment has been minimized.

Copy link
Contributor

nathany commented Jan 31, 2017

When running dep init on a project previously managed with gvt, my hope would be to read in the gvt vendor/manifest and use the commit sha from that file for the new lock.json file instead of basing it on SHAs in the GOPATH.

That's the only part that I feel needs importing in this case, matching the lock file to what exists in vendor based on metadata from an existing tool.

I'd actually prefer everything else be based on the dep solver, which may find more/less packages than the vendor/ folder or manifest contain.

It's imperfect, but establishes a good base to continue from.

An alternative to building this into dep init or providing stand-alone tools might be to have the other tools write out a dep manifest (once solidified). e.g. gvt export to write the correct manifest for dep.

@nathany

This comment has been minimized.

Copy link
Contributor

nathany commented Jan 31, 2017

On the flip side, if dep init or specific mode of init is focused around using the SHAs from GOPATH or whatever the latest version is from the network, then I think the vendor folder should be updated on init.

The problem I saw was that dep init was locking in commit SHAs that didn't match what was in my vendor folder -- not what was already there, and not updating what was there to match.

"Note: init does NOT vendor dependencies at the moment. See dep ensure." dep init -h

Though I'd personally prefer that the lock file was imported from the legacy tool so I could upgrade dependencies gradually.

@dmitris

This comment has been minimized.

Copy link
Contributor

dmitris commented Feb 1, 2017

I would love for dep init to have an option to pick up the "source" origin (in Glide - repo field such as repo: git@opensource.mirror.tld.com:golang/net.git) from the current GOPATH and insert it into the manifest file so that you would not have to do it manually for each of the many mirrored repos.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Feb 1, 2017

@dmitris that's doable, though it would be a bit of work with the way things are currently split up. Would your use case not be better served simply by being able to convert a glide.yaml?

@dmitris

This comment has been minimized.

Copy link
Contributor

dmitris commented Feb 1, 2017

@sdboyer - that would mean that when starting a new project, I would need to manually add to the manifest the repo or source field to every "external" (github.com, golang.org/x etc.) dependency to point to the mirror (have to do the same thing with glide now as well).

In any case, being able to convert from glide.yaml without loss of information such as repo mappings would of course be very helpful for the existing projects.

@nathany

This comment has been minimized.

Copy link
Contributor

nathany commented Feb 5, 2017

Based on #222, would dep init have a mode that operates transitively on a mix of legacy tool formats to extract constraints and the git SHAs in order to prepare the initial manifest and lock file?

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Feb 5, 2017

@dmitris it sounds like you're trying to automate adherence to an organizational policy around using internal mirrors of external deps. If so, I think that's more the class of problem we're discussing in #174. That discussion doesn't necessarily obviate the case for reading in remote source info for the manifest. But, doing that heads further into "making sense of what's in GOPATH" territory, which is full of landmines. Without a compelling reason to do it, my preference is to try to keep dep separate.

@nathany #222 is about how the solver interacts with metadata from other tools during solving - not the up-front constraint collection that happens in init. I don't see how we could usefully incorporate information from deps' tool files, transitive or otherwise, in constructing the root's manifest. Could you say more about what you're imagining?

@nathany

This comment has been minimized.

Copy link
Contributor

nathany commented Feb 6, 2017

@sdboyer After reading #222, I was imaging this is more complex than I originally thought. 😊 But do any existing libraries have manifests that other tools use for resolving dependencies? And would those need to be considered, or could it be a simpler conversion of manifest formats for a single project? (not transitively)

@rfliam

This comment has been minimized.

Copy link

rfliam commented Feb 9, 2017

I think you will also eventually want a -prompt-conflicting. If we march the graph and find that (for example) a library demands >2.0 and one demands <2.0 there is no proper resolution. But a developer should be able to say "I actually know >2.0" works.

Also the mode I see the most used in canticle is -ondisk. That just says "I know what I have works, save it please". I still warn about conflicts but run blindly forward.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Feb 10, 2017

I think, @rfliam, those ideas start taking us a bit off topic, because the dep init algorithm (on which this issue is focused) is very different from that of dep ensure. So yeah, let's avoid derailing this by discussing that in slack, then potentially opening a separate issue for it.

@sdboyer sdboyer added the Epic label Feb 20, 2017

@sdboyer sdboyer referenced this issue Mar 2, 2017

Closed

(re)Spec and implement command set #277

8 of 8 tasks complete
@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Apr 13, 2017

@sdboyer I am interested in working on dep init so that it takes glide's config files into account. Sounds like you've already done some initial work in glide(?), so wanted to check in before making an issue for this and diving in.

Selfishly I'd like to config the CI builds for my own projects (like dvm), and have dep handle populating my vendor directly from the glide config files. As a stopgap until checking in the manifest/lock is encouraged and I can go whole hog on dep proper.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 14, 2017

@carolynvs seems reasonable :)

What I'd done before was a more comprehensive refactoring of the glide manifest and lock format/struct itself to comply with the gps.Manifest and gps.Lock interfaces. That's not directly useful in and of itself, because our focus is not changing glide's own file formats.

However, I did write autoconversion logic from glide's current structs to that new structs. It's should be reasonably straightforward to swap in our manifest/lock structs for the new glide ones there.

The conversion does have some failure modes, but IIRC it works pretty well overall.

kris-nova pushed a commit to kris-nova/dep that referenced this issue Apr 21, 2017

Merge pull request golang#186 from spenczar/check_for_executables_pre…
…test

Check that git, bazaar, and mercurial are installed before running tests that require them
@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Jun 6, 2017

Now that the first importer is in, we are ready for brave volunteers to tackle the others!

Here's how to go about adding support for importing config from another dependency manager during dep init:

  1. Create an unexported importer struct for the tool in cmd/dep/toolbinary_importer.go, e.g. glideImporter in glide_importer.go.
  2. Implement the importer interface:
  • Name() string should return the name for the tool's binary, e.g. glide.
  • HasDepMetadata(dir string) bool should return true if the project in the specified directory contains configuration files for the tool. Not all tools will place their config in the root directory of a project, it's okay to look in a subdirectory to find the config.
  • Import(path string, pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, error) is called when HasDepMetadata returns true. It should read the configuration files for the tool, returning a dep Manifest, and optionally a dep Lock if enough information is available for that as well.
  1. Put unit tests in cmd/dep/toolbinary_importer_tests.go and test configuration files in cmd/dep/testdata/toolbinary/.
  2. Put integration tests in cmd/dep/testdata/harness_tests/init/toolbinary/. You don't need to test the -skip-tools flag.
  3. Add to the supported list of tools in docs/FAQ.md and init.go.

When working on the Import function, here are a few things to note:

  • Populate the manifest's Constraints and Ignored fields. Don't populate Required or Overrides.
  • There is a helper function sm.InferConstraint which will convert a string into a constraint suitable for the manifest. It should do all the heavy lifting for you. For example:
    • v1.3 becomes a semantic version constraint ^1.3.0.
    • An empty constraint becomes a branch constraint for the default branch, such as master.
    • Recognizes branch and tag names.
    • Recognizes a SHA1 (git/hg) or Bazaar formatted revision.
  • dep currently doesn't distinguish between regular and "test/dev" dependencies. If the tool's config contains information for test/dev dependencies, treat them in as regular dependencies.
  • Projects in the lock are created with gps.NewLockedProject(pi gps.ProjectIdentifier, v gps.Version, nil). Do not populate subpackages, as they will be calculated later during solve.
  • There are helper functions for creating the gps.Version to pass into NewLockedProject which should do all the heavy lifting for you. A locked project must have a revision, but ideally it also has the branch/tag represented by that revision as well, so we can easily tell not only the revision but version we are locked into.

As I wrote this, I realized it all belongs as inline doc, so I'll add that hopefully very soon! 😊

@bpicode

This comment has been minimized.

Copy link

bpicode commented Aug 12, 2017

@carolynvs I suggest adding https://github.com/govend/govend. Writing an importer should be relatively straightforward.

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Aug 12, 2017

@bpicode Thanks! I've added #998 and flagged it as up-for-grabs.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Aug 28, 2017

@bpicode just doing some queue cleaning, and i'm wondering if this issue is actually serving much of a purpose anymore. at minimum, do we have an answer for when would we consider it "done"?

@bpicode

This comment has been minimized.

Copy link

bpicode commented Aug 28, 2017

cc @carolynvs, assuming the question of @sdboyer was directed to you ;)

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Aug 28, 2017

This kind of turned into an epic, but really there's no need for it to stay open while people work on the remaining importers. Closing...

@carolynvs carolynvs closed this Aug 28, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Aug 28, 2017

lol sorry @bpicode yes, i must've been tired last night :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment