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

Issue #28: Convert build to using gb #29

Conversation

leifmadsen
Copy link

Use http://getgb.io/ as the builder. Allows for vendoring imports to control the versions in use (no surprises when building).

Makes the build process a bit easier too. Once you install gb, you just need to run: gb build which will result in a binary in the bin/ directory.

Purpose of this pull request to see if the maintainer of the project likes this approach. If so, I can keep working through some of the other things that are required to make this work, such as:

  • documentation updates
  • update / remove the Makefile (as necessary)
  • update wercker.yml to build using gb (may require changes to the base docker image? Probably just needs gb installed on it, then updated yml file to run specific commands)

Anything else I'm missing?

Leif Madsen added 3 commits November 9, 2015 10:05
Had to move around some files and rename some namespaces
in order to not conflict. These files that were conflicting
are not located in the src/gohan/ directory and imported as
gohan/<foo> where foo is the existing package name.
@leifmadsen leifmadsen mentioned this pull request Nov 9, 2015
@nati
Copy link
Contributor

nati commented Nov 9, 2015

Thanks! looks nice!

It look really nice to use local path. I faced some issue when we working on different repo, so I'm really happy it get solved by this PR.

  1. Could you rebase 3 commit to 1 commit?
  2. Godeps dir isn't needed, right?
  3. I wanna keep Makefile.

@leifmadsen
Copy link
Author

For sure, I was planning on rebasing back down to a single commit, just didn't want to spend a bunch of time, and wanted to make it obvious what changes happened. I will flatten the commit history.

Correct, Godeps is no longer needed, as it is handled via the manifest file in vendor/

Sure, I'll update the Makefile for you to use gb.

I'll start looking at updating stuff on wercker as well. Might need some changed in the Dockerfile that is built and used as a container image. I can start looking at that as well. I think the wercker.yml changes will take the longest.

What is the best way to test that wercker will success with my changes? I haven't done much with it before.

@nati
Copy link
Contributor

nati commented Nov 9, 2015

👍

You can test werker file by pushing new version for this PR.

You can take a look result here.
https://app.wercker.com/#applications/5616cd90d77c55dc730caaff

It looks like we have way to run werker locally, but I havn'et tried yet.

@leifmadsen
Copy link
Author

@nati haha nice, I basically came to the same conclusion that it would probably run each time I did a pull request. Just working on some Docker building stuff for another project, but will come back later today or tomorrow. I'll try and move this through the process as quickly as possible so that keeping it in sync with the main repo isn't a pain.

Basic rebases won't work because of how much has been moved around and changed I suspect.

@nati
Copy link
Contributor

nati commented Nov 9, 2015

@leifmadsen Yep. We may have another PR get merged before then. (It could be one from #30 )..

@nati
Copy link
Contributor

nati commented Nov 10, 2015

Hi

I could update Makefile
https://github.com/nati/gohan/tree/feature/issue-28_convert_to_gb_build

We need research..
(1) how to get code coverage
(2) how to pass ginkgo param for gb test
(3) cross compile

@leifmadsen
Copy link
Author

@nati I looked at your Makefile changes, and those look legit. I'm thinking in order to make sure we don't get too out of sync with each other, that we merge this down into the branch, since it isn't master. Then we can merge your Makefile changes.

I'll start working on the wercker and Dockerfile (if necessary) to move that part forward.

@leifmadsen
Copy link
Author

Should we open separate tasks for those 3 items you mention? We could work them independently. I don't think they should be too difficult.

Additionally, I don't think you need to cd src for the gb commands, as it assumes the base code is in the src/ directory. I haven't tested yet though to see if removing cd src/ works.

nati added a commit that referenced this pull request Nov 11, 2015
@nati nati merged commit a7816e4 into cloudwan:implements_issue_28 Nov 11, 2015
@nati
Copy link
Contributor

nati commented Nov 11, 2015

Thanks! i've merged commit to cloudwan:implements_issue_28
let's keep track about this issue in #28

i've tried gb test without "cd src", but i got no lack..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants