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

dep: Permit no Go source for ensure -add #1325

Closed
wants to merge 2 commits into from
Closed

dep: Permit no Go source for ensure -add #1325

wants to merge 2 commits into from

Conversation

jackwilsdon
Copy link

@jackwilsdon jackwilsdon commented Oct 28, 2017

What does this do / why do we need it?

Allows a directory to contain no Go code when running dep ensure -add

Do you need help or clarification on anything?

Is this the best way to do it? I feel like there may be a better way than setting noGoErrors to 0 - should I not increment noGoErrors if allowNoGoErrors is true? Or maybe change both of the if statements which depend on noGoErrors to check allowNoGoErrors too?

Does a new test need creating to cover this?

Which issue(s) does this PR fix?

#1312: dep ensure -add should not depend on Go code

@jackwilsdon jackwilsdon requested a review from ibrasho as a code owner October 28, 2017 22:23
@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.

@jackwilsdon
Copy link
Author

I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

ah, thanks for taking this on!

adding a new param is a bit of a hack, but it's not the end of the world. we definitely need a test for this, though! something under cmd/dep/testdata/harness_tests.

if allowNoGo {
noGoErrors = 0
}

// If pkgtree was empty or all dirs lacked any Go code, return an error.
if len(m) == 0 || len(m) == noGoErrors {
Copy link
Member

Choose a reason for hiding this comment

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

the only problem is the len(m) == 0 case. it's not likely to be reachable under normal circumstances - the directory Gopkg.toml is in will always be scanned, so there's always going to be at least one - but it's probably possible to hit it with ignores. so, both checks will need modification.

Copy link
Author

@jackwilsdon jackwilsdon Nov 27, 2017

Choose a reason for hiding this comment

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

Fixed in 40f0f41.

@sdboyer
Copy link
Member

sdboyer commented Nov 14, 2017

bump - anything i can do to help this along?

@jackwilsdon
Copy link
Author

Sorry for the delayed reply! Just came back from holiday 😎.

Just had a thought - instead of completely forgetting about the "no dirs contained any Go code" error, is it worth instead returning that as a non-fatal error?

Regarding the test case - am I best putting this in cmd/dep/testdata/harness_tests/ensure/add/errs/no-go, and copying how cmd/dep/testdata/harness_tests/ensure/pkg-errors/case1 works?

@jackwilsdon
Copy link
Author

jackwilsdon commented Nov 27, 2017

Just noticed that @arbourd commented on build errors also causing dep ensure -add to fail, is that something else that needs changing?

dep ensure -add won't work if the project can't build, either:

all dirs contained build errors

@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.

4 participants