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

Don't add unconstrained deps to Manifest on init. #585

Merged
merged 2 commits into from May 18, 2017

Conversation

Projects
None yet
4 participants
@jonstacks
Copy link
Contributor

jonstacks commented May 16, 2017

Attempts to fix #574.

Ran dep ensure before I built this locally to get dependencies. It added all of the vendor files and not sure if they should be there.

Also, I wasn't sure if this was better suited being moved into the gps package instead of just in the init.

Thanks,

@googlebot googlebot added the cla: yes label May 16, 2017

delete(m.Dependencies, pr)
}
}

This comment has been minimized.

@darkowlzz

darkowlzz May 16, 2017

Collaborator

Nice, this would do. But I see projects are added to manifest from projectData's constraints here, which gets filled up here. I think if you perform a similar check on the value returned by getProjectPropertiesFromVersion(), we can avoid adding unconstrained projects to manifest at all.

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 16, 2017

Ran dep ensure before I built this locally to get dependencies. It added all of the vendor files and not sure if they should be there.

I don't think it's required. The vendor projects are also tracked in git. But I do see some changes in vendored project code in the PR, which could have appeared due to running some form of ensure. Are you sure you ran just dep ensure? I ran the same but didn't get any such changes in vendor.

// Find any solved dependencies that are unconstrained and remove them from
// the manifest.
for pr, d := range m.Dependencies {
if d.Constraint == nil {

This comment has been minimized.

@sdboyer

sdboyer May 16, 2017

Member

Also need to check if d.Source == "" - we only consider a project empty/unconstrained if it has neither a version constraint, nor a special source location.

@jonstacks

This comment has been minimized.

Copy link
Contributor

jonstacks commented May 16, 2017

@darkowlzz, I'm not entirely sure thats all I ran. It could be because I messed up when I cloned it to $GOPATH/src/github.com/jonstacks/dep before I realized none of the imports would work. Pretty sure I ran dep ensure there and then just moved it to the correct location. I'm assuming that would probably explain the issue. I'll go ahead and remove those changes to the vendor directory. Thanks for the great PR feedback

@jonstacks

This comment has been minimized.

Copy link
Contributor

jonstacks commented May 16, 2017

@darkowlzz, Was able to remove those vendor changes just fine. On a different note, is it expected that this change would also change the lock file's memo? My guess is that It shouldn't change it since it is a hash of the lock files dependencies.

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 16, 2017

is it expected that this change would also change the lock file's memo?

@jonstacks yes, it's expected to change. Memo is calculated based on manifest and lock data. You can run dep hash-inputs and see all the inputs that determine the value of memo. So, if your manifest or lock got changed, since the generation of the first memo, you would get a different memo value.

I think a git hard reset to the current latest (with git clean, if required) would get rid of all the manifest, lock and vendor changes. And then you can switch to your feature branch (rebase if required) and continue working, provided you remove any manifest, lock and vendor changes form your old commits. :)

@jonstacks jonstacks force-pushed the jonstacks:master branch from 814607a to 452b95d May 16, 2017

@jonstacks jonstacks force-pushed the jonstacks:master branch from 452b95d to c4a1c67 May 17, 2017

@jonstacks

This comment has been minimized.

Copy link
Contributor

jonstacks commented May 17, 2017

@sdboyer,

I have made changes to make sure that if either the Constraint or Source exist then it will get added. Thanks to both you and @darkowlzz for helping me out.

} else {
if ctx.Loggers.Verbose {
ctx.Loggers.Err.Printf("dep: \"%v\" is unconstrained. Not adding to project data.\n", pr)
}

This comment has been minimized.

@darkowlzz

darkowlzz May 17, 2017

Collaborator

This logging is not required. We are trying to remove the existing "dep: ..." logs at init because it makes the init output (verbose/non-verbose) confusing, combined with constraint feedback output.
The feedback() following this code would handle displaying that it's a revision only project constraint.

This comment has been minimized.

@jonstacks

jonstacks May 17, 2017

Contributor

It has been removed.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 18, 2017

LGTM - yay, first contributions! 🎉 🎉

@sdboyer sdboyer merged commit 10aeca6 into golang:master May 18, 2017

4 checks passed

cla/google All necessary CLAs are signed
codeclimate no new or fixed issues
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment