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

project structure: move gps into a dep internal directory #300

Closed
kardianos opened this Issue Mar 7, 2017 · 43 comments

Comments

Projects
None yet
9 participants
@kardianos
Copy link

kardianos commented Mar 7, 2017

Right now development is in two places: dep and gps. Also gps has to satisfy two customers: dep and glide.

If it is a goal of dep a) become the standard and b) be developed quickly, I suspect pulling gps into an internal directory for now, allowing the API to change quickly and atomically, would speed up development. It would also put all issues in a single location. Let glide continue to use the gps version where it currently is.

To be clear, I applaud breaking up the tool into an package API and a CLI. That is exactly how govendor is structured. And in time the interna/gps package would be removed from internal to an exposed package.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Mar 7, 2017

I feel like this has been raised before, but I can't find it in the queue...maybe it was in slack, or something.

glide isn't actually using gps right now - the integration branch that I was working on hasn't moved in months. So the two customers thing isn't really an issue (though the requirements haven't really diverged, either).

It's not totally clear to me that having gps in a separate repository is something that's really an impediment to development. But, my gut reaction to this proposal right now is that it'd slay something I've worked on for a year, so my judgment isn't the clearest.

So: if we really think it would speed up development, then sure, let's do it. Migrating the (open) issues would also definitely be needed, and would be a bit of work - help on that would be great. I'd also rather not put it in an internal directory. It seems like that would just encourage us to muddy the line between the library and the tool. Plus - let's not pretend, here - moving gps into dep directly would kill sdboyer/gps, and I don't want a public interface to go away.


EDIT - this comment was really not well-thought through. Obviously, gps has to move into dep sooner or later. That, of course, means some loss of some control for me - but that's kinda the whole idea here. I responded before having really overcome my gut reaction, and I'm sorry.

@kardianos

This comment has been minimized.

Copy link

kardianos commented Mar 7, 2017

@sdboyer Developing golang/dep is a proposal to kill govendor, gvt, etc, many of these have been in development for over a year or more. If the API package is central to dep, then the API package also becomes at least somewhat official. If it is an official API package, we need to be careful not to break API, or carefully version the API.

What I'm proposing is that the API package should be unstable as dep is still pre 1.0 and many features need to be added. As such it should be in an internal directory by definition and by go std definition.

I mean what I'm saying is I don't want to worry about breaking API or versioning if I were to personally contribute. I'm willing to contribute to golang/dep (provided I can make the time), but I don't have the will to contribute to sdboyer/gps at this time, esp when you say you don't want the public interface to go away, I don't want to worry about an existing public interface at this stage.

I suppose I would see if there are others in a similar boat as I'm in.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Mar 7, 2017

Developing golang/dep is a proposal to kill govendor, gvt, etc, many of these have been in development for over a year or more.

Sure, of course, we're all putting our projects/work on the line here. Sorry, I didn't mean to suggest otherwise - I'm just being honest about my gut reaction to the proposal, and explaining why my judgment about its utility is clouded.

What I'm proposing is that the API package should be unstable as dep is still pre 1.0 and many features need to be added. As such it should be in an internal directory by definition and by go std definition.

We're not adhering to that within the github.com/golang/dep package itself, right now. Which should be fine, because the dogfood we're eating here - at least, prior to merging into the toolchain and becoming subject to different guarantees - is the model this tool enables, not the model as it has been. That is, we (or at least gps) are following semver with pre-1.0.0 releases, which provide no guarantee of API stability. v0.14.0 of gps broke v0.13.0, which broke v0.12.0. Which is to say:

esp when you say you don't want the public interface to go away, I don't want to worry about an existing public interface at this stage.

I'm not worrying about it, and IMO, you needn't be, either. But, yes -

I suppose I would see if there are others in a similar boat as I'm in.

I think it'd be good to get more feedback, too.

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Mar 7, 2017

gps solves an interesting problem and I would hate to see it end up hidden in an internal package that only dep can use.

I'm working on PR right now that crosses the boundary between the two repos. It's a little more work but personally, having them clearly separated reinforced that it's a public interface, so I wasn't tempted to muck about with it willy-nilly.

Just throwing in my 2 cents, that having gps separate hasn't been a burden to this contributor at all.

@kardianos

This comment has been minimized.

Copy link

kardianos commented Mar 7, 2017

(I'm not suggesting gps be internal for all time, just during primary development.) That's just it, we need to be able to muck with it willy-nilly during this period. That's exactly my point.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Mar 8, 2017

OK, I conferred with other committee members, and they're inclined to move it now, so let's do it. I'll get the remaining PRs finished up, then pull it over.

The big blocker now, then, is migrating the existing issues. I'd really like to avoid doing that by hand, if possible :)

@kardianos

This comment has been minimized.

Copy link

kardianos commented Mar 8, 2017

Issue migration might be assisted with: https://github.com/IQAndreas/github-issues-import or similar.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Mar 13, 2017

I'm gonna work on making this happen this week

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Mar 16, 2017

OK so, after reading around about licensing, I'm now more confused than when I started.

Does anyone understand what the necessary steps are, license-wise, to moving gps into dep directly? @adg?

@adg

This comment has been minimized.

Copy link
Contributor

adg commented Mar 23, 2017

I've enquired with our open source people, and hope to hear from them soon.

@adg

This comment has been minimized.

Copy link
Contributor

adg commented Mar 30, 2017

So there are two options:

  1. get each one of gps' 13 contributors to sign our cla (cla.developers.google.com), and get them to warrant over email that they agree to a license change, and that their employer agrees too.
  2. commit gps to the repo in a vendor directory with its original LICENSE file.

Option 1 doesn't seem too onerous to me, but I'm not familiar with all of the contributors. What do you think?

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Mar 30, 2017

I think option 1 is probably possible. Who needs to be involved with the emailing?

Option 2 is what we're doing now. If we were to try to work on it within vendor/...well, that would be an interesting conundrum, because hacking on something in vendor/ is expressly forbidden by the way dep works :)

@adg

This comment has been minimized.

Copy link
Contributor

adg commented Mar 30, 2017

@omeid

This comment has been minimized.

Copy link

omeid commented Mar 30, 2017

In the unlikely event that option 1 doesn't go through, you should be still able to vendor a fork of gps, that way no direct mucking around with vendor and still afford large and breaking changes fast directly the to fork. The upstream may want to merge, but dep can move on fast regardless.

@zellyn

This comment has been minimized.

Copy link
Contributor

zellyn commented Apr 13, 2017

As one of the most important contributors to GPS :-p, you have my permission to do whatever you want: I always saw it as your code.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 13, 2017

/me bows to @zellyn ❤️

@adg let me know that all we need is an issue on gps in which all the contributors consent to the change, and we can go ahead. Making that issue now...

@zellyn

This comment has been minimized.

Copy link
Contributor

zellyn commented Apr 13, 2017

Thanks, @sdboyer! Please point me to that issue so I can describe the towering majesty of my contributions in increasingly hyperbolic terms.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 13, 2017

here 'tis: sdboyer/gps#215

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 15, 2017

OK, we've got the 👍 from everybody over there - seems like we can start the process of moving it over.

Not sure if I'll have time to do that this weekend, or not. I Also wouldn't mind if someone else were to tackle it 😄

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 16, 2017

OK, we've got the go-ahead from everyone. I'll look at what's involved in the issue migration this week.

@kardianos

This comment has been minimized.

Copy link

kardianos commented Apr 17, 2017

@bradfitz has been fixing up the gopherbot recently.

@rakyll

This comment has been minimized.

Copy link
Member

rakyll commented Apr 18, 2017

@bradfitz or @rsc can help with the gopherbot.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 21, 2017

@kris-nova let's not wait on the bot - just make a new account for this purpose. Maybe with a cute avatar from gopherize.me 😄

@kris-nova

This comment has been minimized.

Copy link
Contributor

kris-nova commented Apr 21, 2017

Word will get on it today 👍🏻

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 21, 2017

@kris-nova sweet! lmk when you have it; if it needs perms for this repo i believe i can provide them

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 26, 2017

@adg next question - what's supposed to happen with the original license file from gps, and what's the deal with license headers on gps files? The copyright hasn't changed, if I understand all this correctly

@adg

This comment has been minimized.

Copy link
Contributor

adg commented Apr 26, 2017

@sdboyer looking at pull request #453, I don't see copyright headers on the files?

I think they should have these kinds of headers, like dep:

// Copyright 2016 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.

and then we'll make sure that the Go AUTHORS list includes the gps authors.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 27, 2017

@adg OK, that wfm - @kris-nova, here's another question answered :)

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 27, 2017

(though s/2016/2017)

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 3, 2017

OK, gps is moved in 😄

What do we need to do to update the Go AUTHORS list?

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 3, 2017

(keeping this open till that last bit is done)

@adg

This comment has been minimized.

Copy link
Contributor

adg commented May 10, 2017

@bradfitz what's the process for updating AUTHORS+CONTRIBUTORS? Is it fully automated yet? Or a thing that you run? Is the dep repo part of it?

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented May 10, 2017

Yes, it's automated, but a Googler needs to run the program because it hits the internal CLA verification service.

@adg

This comment has been minimized.

Copy link
Contributor

adg commented May 10, 2017

@bradfitz Will the contributors to the dep repo be picked up by the script next time it runs?

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented May 10, 2017

If somebody adds a line to the script, yes.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented May 10, 2017

People can also add themselves, since Gerrit checks CLAs now anyway.

@adg

This comment has been minimized.

Copy link
Contributor

adg commented May 10, 2017

@sdboyer sdboyer closed this in 9ea8489 May 10, 2017

ibrasho added a commit to ibrasho-forks/dep that referenced this issue May 10, 2017

ibrasho pushed a commit to ibrasho-forks/dep that referenced this issue May 10, 2017

@sdboyer sdboyer reopened this May 11, 2017

@carolynvs carolynvs added the area: gps label May 31, 2017

@sdboyer sdboyer closed this Aug 10, 2017

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