Adds Golang 1.13.1 as a Software definition #1098
Conversation
e069e58 to
e42cc00
Compare
Signed-off-by: Salim Afiune <afiune@chef.io>
e42cc00 to
ede7b53
Compare
Having the `sync` statement made our omnibus build to complain with the following error: ``` [Builder: libtool] I | 2019-09-27T06:03:20+00:00 | Build libtool: 0.0011s RuntimeError: Can not find config.guess. Make sure you add a dependency on 'config_guess' in your software definition ``` By avoiding that sync we "fixed" the problem. (arg workaround it) Signed-off-by: Salim Afiune <afiune@chef.io>
Signed-off-by: Salim Afiune <afiune@chef.io>
config/software/go.rb
Outdated
| build do | ||
| mkdir "#{install_dir}/embedded/bin" | ||
| %w{go gofmt}.each do |bin| | ||
| link "#{project_dir}/go/bin/#{bin}", "#{install_dir}/embedded/bin/#{bin}" |
There was a problem hiding this comment.
I think you'll want to do a copy or sync here - otherwise you'll be linking from the "build" directory to the installation directory, resulting in failed linking when it's packaged up.
There was a problem hiding this comment.
Thanks @scotthain for the review. As you can see in the commit history, I originally did a sync but it was causing failures (reference: 3fc04fc). Another thing to consider is that we need Go to compile the binaries we will be developing but we don’t need to package it. So probably we will delete these links.
There was a problem hiding this comment.
Ah in that case I don't think it belongs in omnibus-software since your use case is slightly different. Since you'll be deleting the links in (I assume) a separate, private (by that I mean not-in-omnibus-software) "cleanup" definition, this will break if anyone assumes it "installs go". @chef/omnibus-maintainers any thoughts on this?
There was a problem hiding this comment.
I was going to add that cleanup in this repo, I am not quite done with this PR, I’m still doing compile tests. I added it here because Rust is also here and I thought it would be a good place to add Go, but if this is not the correct repo, where should you recommend I should add it?
There was a problem hiding this comment.
So the rust def and corresponding uninstall is slightly different because it actually does a full install of rust to the install_dir and then does a full uninstall. The way I see it there are 2 options:
- Do a full install of go in this def and create a
go-uninstalldef that does a full remove. This allows folks who actually want to include/have a "permanent" go installation work correctly when using this def. (and a corresponding uninstall) - Put the def as is (with the linking) in the project that uses it, and remove the links in a cleanup def in the project that uses it.
Happy to use mouthwords to talk more about this if I'm not being clear.
There was a problem hiding this comment.
Sounds good to me, I'll change things to do a full install. Thanks for the review Scott! ❤️ 🎉 🥇
There was a problem hiding this comment.
Absolutely! Thanks for being open to the feedback 🚀 🍰
Signed-off-by: Salim Afiune <afiune@chef.io>
Signed-off-by: Salim Afiune <afiune@chef.io>
|
@scotthain I haven't tested this but, does it look better now? |
|
@afiune looks great! could you run a quick test (add it to omnibus-harmony for a quicker test if you'd like) to make sure it works as expected? |
Signed-off-by: Salim Afiune <afiune@chef.io>
Signed-off-by: Salim Afiune <afiune@chef.io>
Depends on chef/omnibus-software#1098 Signed-off-by: Salim Afiune <afiune@chef.io>
…o afiune/add-golang
Signed-off-by: Salim Afiune <afiune@chef.io>
e191d0f to
730c5bc
Compare
|
Awesome looks great now! |
Depends on chef/omnibus-software#1098 Signed-off-by: Salim Afiune <afiune@chef.io>
Description
Adds Golang as an Omnibus Software definition.
Related Issue
chef/chef-workstation#497
Types of changes
Checklist: