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

More or less complete init (still with stubs) #18

Merged
merged 10 commits into from
Dec 2, 2016
Merged

More or less complete init (still with stubs) #18

merged 10 commits into from
Dec 2, 2016

Conversation

sdboyer
Copy link
Member

@sdboyer sdboyer commented Dec 1, 2016

This brings init to rudimentary completion. There are two new, noteworthy stub methods in here, plus addition the one introduced in #14. The work to flesh those out isn't trivial, but at least it's now pretty well-defined.

We also still need to actually write out a vendor directory, but I think that should be just one more gps function call. See #19

init now scans the code tree of the project it is init-ing and attempts
to make a sane determination about what constraint it should apply for
each non-stdlib project root it encounters.

This adds another handwavy stub method - this one for figuring out what
version of code is currently on disk - but it illustrates intent, and we
can fix that later.
init now creates a provisional lock file from the version information it
inferred while scanning the workspace. The provisional lock is used to
bootstrap a solve run that will:

 * Retain the versions given in the provisional lock
 * Pick versions for any direct deps that were in the import graph, but
   not found in the workspace
 * Pull in and pick versions for any needed transitive deps according to
   the rules in dep's analyzer (currently very dumb)

This logic relies on another new stub function to handle the huge
variety of failures that solving can have; that's going to be a
significant and ongoing area of work.

We're still not writing out the vendor directory just yet, though that
should be just one more gps function call.
@@ -71,18 +70,24 @@ func runInit(args []string) error {
}

mf := filepath.Join(p, manifestName)
lf := filepath.Join(p, lockName)

// TODO: Lstat ? Do we care?
_, err = os.Stat(mf)
if err == nil {
return fmt.Errorf("Manifest file '%s' already exists", mf)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use %q here also

if err == nil {
return fmt.Errorf("Invalid state: manifest %q does not exist, but lock %q does.", mf, lf)
}

if os.IsNotExist(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we're testing for an IsNotExist error here, but only for the lock file.
I think we should have a helper func exists(name string) (bool, error) and use that for both these

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, this was actually a mistake that crept in - that if branch was supposed to apply to the manifest's stat error, not the lock's. It needs to be moved under there, anyway. That may obviate the need for a separate func?

@@ -123,16 +209,38 @@ func isStdLib(i string) bool {
return false
}

func writeManifest(path string, m rawManifest) error {
// TODO rename this to something not dumb when it becomes not a stub
func whatVersionIsOnDiskForThisThing(pr gps.ProjectRoot) (gps.Version, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please just name it versionOnDisk

Copy link
Member Author

Choose a reason for hiding this comment

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

i went for versionInWorkspace instead, i think that's a bit more precise

sdboyer and others added 6 commits December 1, 2016 08:45
Signed-off-by: Jess Frazelle <acidburn@google.com>
This introduces a depth-first traversal of the import graph; gps can't
help, because we're exploring a GOPATH workspace, which gps explicitly
does not do.
@sdboyer
Copy link
Member Author

sdboyer commented Dec 2, 2016

After @freeformz and I finished up last night, I realized that we'd made a poor choice by dealing with the current project's deps by scanning the workspace, but relying on a gps solve run to deal with transitive deps. If the goal of [this mode of] init is really to rely as much as possible on reading from the workspace, then we needed to do that for transitive deps as well.

So, that's what @jessfraz and I focused on in pairing. And it got kinda hairy, unsurprisingly - we had to write a custom depth-first traversal algorithm to make it all work. But...seems like we pulled it off! 🎉 🎉 🎉

We finished literally minutes before Jess had to leave for the bus, so we didn't have time to write tests for this...and we're almost certainly going to want to have some of those for something this complex. That won't be terribly easy, of course, because this is pretty hardcoded to disk reads...but hey, that's a problem for later.

@jessfraz
Copy link
Contributor

jessfraz commented Dec 2, 2016

yay!! LGTM

@freeformz freeformz merged commit d4cbaa4 into golang:master Dec 2, 2016
zbintliff added a commit to zbintliff/dep that referenced this pull request Mar 3, 2017
More or less complete init (still with stubs)
krisnova pushed a commit to krisnova/dep that referenced this pull request Apr 21, 2017
ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017
More or less complete init (still with stubs)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants