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

[WIP] Add project initialization warnings to dep init cmd #798

Conversation

wslulciuc
Copy link

@wslulciuc wslulciuc commented Jul 3, 2017

Note that I still need to write tests for my solution.

What does this do / why do we need it?

To provide general guidance around new project initialization using the dep init command. That is, display warnings to the user when initializing a project in a project subdirectory, or within a subdirectory of an already initialized project.

For example, given the following project structure:

$GOPATH/src/github.com/myuser/myproject/
├── A
└── B

Running dep init in A/ will be print out the following warning to the console:

$ dep init
WARNING: it is recommended that project initialization be performed at the project root, not a project subdirectory.
...

If we were to run the command in A/ in an already initialized project, the following warning(s) will be observed:

$ dep init
WARNING: found manifest file in another directory.
WARNING: it is recommended that project initialization be performed at the project root, not a project subdirectory.
...

What should your reviewer look out for in this PR?

Which issue(s) does this PR fix?

Fixes #193

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@wslulciuc
Copy link
Author

I signed it

@wslulciuc wslulciuc force-pushed the issue-193/dep-init-subdirectory-warn branch from e19b78a to e2ee00b Compare July 4, 2017 23:06
@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Jul 4, 2017
@darkowlzz
Copy link
Collaborator

Thanks for an initiative to fix/discuss this issue.

I'm not sure if we should warn or do anything when dep is run in a sub directory. I see dep works similar to git, which too doesn't care about creating a git repo inside an existing one. Doing dep status in any sub directory shows status of the parent project, which is again similar to git. idk, maybe some more use-cases/issues would make it more convincing. Maybe @sdboyer @carolynvs @ibrasho would have some better ideas about it?

If we do it, and considering the PR, how about printing the warning message and ask the user to use a force flag (-f) or something similar to forcefully perform initialization and exit?

@carolynvs
Copy link
Collaborator

@darkowlzz Sam has said previously that dep init must be run at the project root, though files can be moved later into subdirectories under certain conditions.

FYI, I haven't reviewed this PR yet, hoping to get to that today.

@sdboyer
Copy link
Member

sdboyer commented Jul 20, 2017

bump - need me on this?

@carolynvs
Copy link
Collaborator

facepalm Sorry, no this just fell through the cracks on my part...

@wslulciuc
Copy link
Author

@sdboyer : Your input would be helpful! As @darkowlzz mentioned, if our goal was to mirror the behaviors of git, then we shouldn't assume the intent of the user (more specifically, that the current directory is not the intended root directory). But given that dep init doesn't have to necessarily be run in the project root (though recommended), I think the introduction of a flag (-f) to force initialization sound reasonable.

@carolynvs
Copy link
Collaborator

carolynvs commented Jul 20, 2017

Sorry that I took so long to review this!

This PR is solving multiple things: determining if we are running dep init is a subpackage instead of the root package, and also if dep has been run before in another directory. The original issue (#193) was only about the former.

I am inclined to not check for existing dep config in subdirs upon init, @sdboyer?

Just looking at how we can tell if we are running dep init in a subpackage, I think it can be as simple as comparing:

  • the output of ctx.ImportForAbs(root) which we already call in init to get the import path based on the current absolute path
  • the output of sm.DeduceProjectRoot(ip) which attempts to figure out the root project from the import path, returning "" if it can't figure it out (which can happen when it's not a predictable path like github.com/golang/dep

If we can determine a project root, and it doesn't match the import path, then we should display a warning that this init may not work, but then try anyway. The warning you have in this scenario seems just right. I really don't want to introduce a force flag, keeping it only as a warning that can help the user just in case the init didn't go right.

@sdboyer
Copy link
Member

sdboyer commented Jul 26, 2017

if our goal was to mirror the behaviors of git

i don't think of this is as actually being our goal - i just use it as a convenient shorthand when explaining to people roughly what dep init does.

we do need to be able to facilitate initializing a project not at a repo root, for a variety of reasons (including some that @carolynvs and i heard about yesterday on a call with k8s folks). i'd also prefer to avoid a -f flag. but it is reasonable to issue a warning, if we can unambiguously figure out that we're not at a repo root.

@mvdan
Copy link
Member

mvdan commented Sep 4, 2020

Dep was officially deprecated earlier this year, and the proposal to archive this repository was accepted. As such, I'm closing outstanding issues before archiving the repository. For any further comments, please use the proposal thread on the Go issue tracker. Thanks!

@mvdan mvdan closed this Sep 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error handling when running dep init from project subdirectory
6 participants