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

Build for static V8 library on Windows #102

Merged
merged 4 commits into from
Apr 9, 2021

Conversation

cleiner
Copy link
Contributor

@cleiner cleiner commented Apr 3, 2021

This PR adds the bits required to provide a prebuilt static V8 library on Windows, offering basically the same level of end user convenience as on Linux/Mac (i.e. not having to deal with a dozen DLLs in a distribution).

Existing Windows builds using v8go should not break, the change merely renders the need to package additional files with the executable unnecessary.

@codecov
Copy link

codecov bot commented Apr 3, 2021

Codecov Report

Merging #102 (397b708) into master (25d7afa) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #102   +/-   ##
=======================================
  Coverage   96.61%   96.61%           
=======================================
  Files          12       12           
  Lines         414      414           
=======================================
  Hits          400      400           
  Misses          9        9           
  Partials        5        5           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25d7afa...397b708. Read the comment docs.

@neptoess
Copy link
Contributor

neptoess commented Apr 5, 2021

@cleiner ,
This is pretty cool, but I'm wondering if it makes more sense to try and get this merged into MSYS2, so everyone using their package has access to a static libv8.a, not just people using v8go. This also removes the need for this repo to track MSYS2's patches, and makes bundling the static library a simple copy command.

@cleiner
Copy link
Contributor Author

cleiner commented Apr 5, 2021

@neptoess,
I had similar thoughts along the way, but more in terms of asking the MSYS2 maintainers if they had tried submitting their patches to Chromium/V8, as it did support MINGW in the past, before the build system was migrated to GN (though I'm a little concerned this will turn out to be kind of a long term endeavor, if at all feasible).

With regard to merging this into MSYS2, I think there's nothing to merge here (except for the tiny 0002 patch) -- it would be more like asking for an additional package built to v8go's needs:

  • Changing the existing package would be breaking for the people currently depending on it
  • ICU is included in the MSYS2 build, but not in v8go; adding it would make the binary too big to bundle
  • v8_untrusted_code_mitigations is enabled for MSYS2, but not for v8go (don't know about the consequences of changing this)
  • I think the component build was chosen deliberately, so people can create their own snapshot_blobs

As I understand it, the need to track the MSYS2 package already exists (and would continue to exist even with a provided libv8.a), because if v8go uses a newer V8 version than MSYS2, Windows builds may break as the V8 API is not guaranteed to stay compatible, is it?
If this is the case, then versions shouldn't diverge and copying the patches will be one additional step in the "Upgrading the V8 binaries" section of the readme. There'd be no need to "patch the patches" as I did here.

That said, I get that it's a potential maintenance burden in its current form and I'd understand if the project would rather not take it on.

@neptoess
Copy link
Contributor

neptoess commented Apr 5, 2021

Good info @cleiner. I agree this is the path to take. I'll leave it up to @rogchap and the other maintainers to determine whether they want to bundle a static lib for Windows. I personally think it's a good idea though.

@rogchap
Copy link
Owner

rogchap commented Apr 6, 2021

@cleiner WOW This looks amazing 🤩
Will make so many Windows users happy.
Will take a closer look this afternoon.

@rogchap
Copy link
Owner

rogchap commented Apr 8, 2021

@zwang Do you have any time to look at this one, as you are more familiar with the Window ecosystem than I?

I want to make sure the path to upgrading V8 to latter versions will be possible/easy/documented, which is my main concern.

@rogchap
Copy link
Owner

rogchap commented Apr 9, 2021

@cleiner can you possibly update with latest master? For some reason GH is returning a 500 when I try and use the GUI to try and update.
I've fixed the FOSSA issue, so the tests should now run successfully.

@zwang
Copy link
Collaborator

zwang commented Apr 9, 2021

@zwang Do you have any time to look at this one, as you are more familiar with the Window ecosystem than I?

I want to make sure the path to upgrading V8 to latter versions will be possible/easy/documented, which is my main concern.

The overall changes looks good to me. Although I could not verify the correctness of the patches, I assume the patches are good since they are from mingw.

Regarding the path to upgrading V8 to latter versions, it seems to be possible and is well documented as I see.

cleiner and others added 4 commits April 9, 2021 16:57
Co-authored-by: cleiner <cleiner@users.noreply.github.com>
Co-authored-by: cleiner <cleiner@users.noreply.github.com>
@cleiner cleiner force-pushed the windows-static-v8-lib branch from 4f7d82e to 397b708 Compare April 9, 2021 15:46
@cleiner
Copy link
Contributor Author

cleiner commented Apr 9, 2021

@cleiner can you possibly update with latest master? For some reason GH is returning a 500 when I try and use the GUI to try and update.
I've fixed the FOSSA issue, so the tests should now run successfully.

@rogchap Rebased on current master.

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.

4 participants