Skip to content
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

WIP: use rogpeppe/godef instead of Manishearth/godef #1005

Closed
wants to merge 2 commits into from

Conversation

timfeirg
Copy link

@timfeirg timfeirg commented Apr 20, 2018

required by ycm-core/YouCompleteMe#2991


This change is Reviewable

@timfeirg timfeirg changed the title use rogpeppe/godef instead of Manishearth/godef WIP: use rogpeppe/godef instead of Manishearth/godef Apr 20, 2018
@bstaletic
Copy link
Collaborator

bstaletic commented Apr 20, 2018

The builds failed because you didn't unregister the old submodule before registering a new one, so git is still looking for the commit from the Manisheart's fork, but now it is looking for it in the upstream repo.

Here's how submodules are changed:

@bstaletic
Copy link
Collaborator

The latest commit should do the job. Let's see if the tests pass.

@timfeirg
Copy link
Author

Building godef failed with dependency issues, see https://travis-ci.org/Valloric/ycmd/jobs/369066405.

I've added godep into build.py to see if it'll work.

@bstaletic
Copy link
Collaborator

There is a ycmd_compat branch on Manisheart's fork and here's what it changed from the defaults:
rogpeppe/godef@master...Manishearth:ycmd_compat

@micbou
Copy link
Collaborator

micbou commented Apr 20, 2018

This fails because the forked godef has custom changes to make it work.

@Manishearth Could you update your godef fork with the latest changes and prepare a PR?


Reviewed 2 of 5 files at r1, 1 of 1 files at r2.
Review status: 3 of 6 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@timfeirg
Copy link
Author

Since godep is an outdated tool, it could be a better idea to migrate from godep to dep for the godef project.
Or just exclude godef from ycm at all, that'd really unload the mess we're having in this PR, and just let user install godef on their own.

@bstaletic
Copy link
Collaborator

I have updated and rebased @Manishearth's changes in my godef fork. The ycmd branch with the updated godef is here.

@timfeirg Can you try my ycmd branch and confirm it fixes the jump to vendor bug?

@Manishearth
Copy link
Contributor

Manishearth commented Apr 20, 2018 via email

@Manishearth
Copy link
Contributor

Bumping the submodule pointer to https://github.com/Manishearth/godef/tree/ycmd_compat (c3b70c6) should be enough. I don't want to break older versions of YCMD so I'll keep that old branch around as ycmd_old and rename the new one.

@bstaletic
Copy link
Collaborator

Thanks @Manishearth! I've opened #1006 to update ycmd's submodule.

@micbou
Copy link
Collaborator

micbou commented Apr 20, 2018

Superseded by PR #1006.

@micbou micbou closed this Apr 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants