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

Support MSYS2 MinGW64 builds #46

Merged
merged 12 commits into from
Dec 17, 2020
Merged

Support MSYS2 MinGW64 builds #46

merged 12 commits into from
Dec 17, 2020

Conversation

neptoess
Copy link
Contributor

@neptoess neptoess commented Dec 16, 2020

These changes will allow v8go to work for users using the MSYS2 MinGW-w64 toolchain (https://www.msys2.org/) and the v8 package in their package manager,
pacman -S mingw-w64-x86_64-v8

Resolves #7, resolves #44

Note that the snapshot_blob.bin file will have to exist in your program's working directory. If the default install paths are used, this file is located at C:\msys64\mingw64\bin\snapshot_blob.bin

@mwm-tesla
Copy link

awesome!

@rogchap
Copy link
Owner

rogchap commented Dec 16, 2020

Amazing @neptoess for getting this working on Windows.

One of the key design decisions of v8go is to include pre-built binaries so that users are able to simply use go get and it "just works".

If we're not able to do this for Windows we'll need to add detailed instructions to the README. Are you able to add this to the PR?

@neptoess
Copy link
Contributor Author

@rogchap Adding prebuilt isn't a great option on Windows, in my opinion, because the default Go installer won't even leave the user with working cgo. Mingw-w64 gcc is the only compiler I'm aware of that allows creating native Windows binaries via cgo, and MSYS2 is a pretty popular way to acquire it, because of how awesome their pacman repo is. It's one extra command for the user to acquire libv8 in addition to gcc. Now, if this package contained a static build of libv8, I would definitely say we should bundle it here. Because it doesn't, shipping a static libv8 for Windows means signing up to build said static library with the Mingw-w64 toolchain. I took a look at the scripts used to build the package (https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-v8/PKGBUILD) and decided that it would probably be significantly more effort to get a static library built than to just dynamically link V8.

tl;dr Yeah I'll modify the README :)

@neptoess
Copy link
Contributor Author

@rogchap Let me know how this looks 458d4c7

@rogchap
Copy link
Owner

rogchap commented Dec 16, 2020

Yup; this is a fair compromise; might also be a better way to provide support for other architectures like freebsd etc. So static binaries for darwin and linux, but dynamic for the others 🤷‍♂️

Copy link
Owner

@rogchap rogchap left a comment

Choose a reason for hiding this comment

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

Thank you. Looks good. Seems I need to fix the CI build (need to merge #6)

README.md Outdated Show resolved Hide resolved
neptoess and others added 2 commits December 16, 2020 17:31
Co-authored-by: Roger Chapman <rogchap@gmail.com>
cgo.go Outdated Show resolved Hide resolved
cgo.go Outdated Show resolved Hide resolved
@rogchap
Copy link
Owner

rogchap commented Dec 17, 2020

Hey @neptoess Thanks for continuing to update this PR.
I've fixed up the test so I could check for any regressions in the PR.
The CI does not include any Windows tests; but we can add that suite later.

This was referenced Dec 17, 2020
@neptoess
Copy link
Contributor Author

Hey @neptoess Thanks for continuing to update this PR.
I've fixed up the test so I could check for any regressions in the PR.
The CI does not include any Windows tests; but we can add that suite later.

I won't sign up to do it here, but I did this with Travis CI for confluent-kafka-go and it wasn't terrible
https://github.com/confluentinc/confluent-kafka-go/pull/564/files

Copy link
Contributor Author

@neptoess neptoess left a comment

Choose a reason for hiding this comment

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

I think I fixed it

@neptoess neptoess requested a review from rogchap December 17, 2020 23:35
Copy link
Owner

@rogchap rogchap left a comment

Choose a reason for hiding this comment

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

All green. Looking good. Thanks again @neptoess

@rogchap rogchap merged commit 2a13784 into rogchap:master Dec 17, 2020
@rogchap
Copy link
Owner

rogchap commented Dec 18, 2020

Published as part of 0.3.0

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.

build error on windowsx64 Include Windows build of V8
3 participants