Skip to content

Use host architecture and OS when running go generate #4492

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.

Sign up for GitHub

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

Jump to bottom
Merged
bk2204 merged 2 commits into from
Apr 29, 2021
Merged

Use host architecture and OS when running go generate #4492

bk2204 merged 2 commits into from
Apr 29, 2021

Conversation

@sergiou87
Copy link
Contributor

@sergiou87 sergiou87 commented Apr 28, 2021

I was trying to cross-compile git-lfs to macOS for arm64 from an x86_64 machine and I noticed that just passing a GOARCH=arm64 variable to make would cause a failure like this:

go generate github.com/git-lfs/git-lfs/commands
fork/exec /var/folders/4d/hy7gsq2n22jccp96wdbpt3100000gn/T/go-build614314336/b001/exe/mangen: bad CPU type in executable
commands/commands.go:28: running "go": exit status 1
make: *** [commands/mancontent_gen.go] Error 1

I don't understand why exactly, but overriding GOARCH with nothing (to effectively use the host CPU arch) for the specific go generate command fixed the issue. However, I don't know if this could cause issues somewhere else or if there is a better way to fix this problem.

Please, let me know what you think 😄

@sergiou87 sergiou87 mentioned this pull request Apr 28, 2021
Merged
bk2204
bk2204 reviewed Apr 28, 2021
View reviewed changes
Copy link
Member

@bk2204 bk2204 left a comment

Choose a reason for hiding this comment

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

Hey,

Thanks for the patch, and welcome to Git LFS!

There are two ways to accomplish what you're looking for here. One of them is to do this and also add GOOS so folks can cross-compile across operating systems.

The other approach is to use the specific Makefile target for those OS/architecture combinations, such as bin/git-lfs-darwin-arm64. These are what we use for building our own releases, and it avoids this problem altogether.

I don't have a strong preference about which way to go; we can take this patch if you're willing to add GOOS here, or, if you don't want to do that, you can just use the existing targets.

@sergiou87
Copy link
Contributor Author

sergiou87 commented Apr 28, 2021

Thanks for the quick reply!

I saw the specific Makefile targets, but dugite was already doing "manual" builds instead of using those targets: https://github.com/desktop/dugite-native/blob/f221234dcd40d00702e43494e4ce5a7a3e271833/script/build-macos.sh#L63

Not sure if there is a problem with them, but I wanted to make the minimum possible changes 😳

I still don’t understand 100% why would GOARCH/GOOS affect go generate like that, specially given it’s only generating manpages content 🤔

So I'd rather go with the other option you suggested, which if I understand you correctly is just adding GOOS= GOARCH= before the go generate command instead of just GOARCH=, is that right @bk2204?

@bk2204
Copy link
Member

bk2204 commented Apr 28, 2021

So I'd rather go with the other option you suggested, which if I understand you correctly is just adding GOOS= GOARCH= before the go generate command instead of just GOARCH=, is that right @bk2204?

Yup, that's definitely what I was suggesting, and I think it should be fine if we do that.

Just as a note, our main branch is in a holding pattern right now. While we can merge this, we're not planning on doing a release until June, which will be our 3.0.0 release (because of the removal of NTLM) and so it may be a while before any change here makes it into a release.

@sergiou87
Copy link
Contributor Author

sergiou87 commented Apr 28, 2021

Yeah don't worry, we also like to be a bit behind the latest version, so I'll just use sed to change the Makefile before building git-lfs, and drop a comment with our conversation here 😄

Thank you!! ❤️

@sergiou87 sergiou87 changed the title Use host architecture when running go generate Use host architecture and OS when running go generate Apr 28, 2021
@sergiou87 sergiou87 requested a review from bk2204 April 28, 2021 15:28
@bk2204
Copy link
Member

bk2204 commented Apr 28, 2021

I'll wait for CI to finish running and then merge this.

@bk2204 bk2204 merged commit 285f77d into git-lfs:main Apr 29, 2021
@sergiou87 sergiou87 deleted the patch-1 branch April 30, 2021 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bk2204 bk2204 bk2204 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sergiou87 @bk2204