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

Many changes to the build system #1351

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Many changes to the build system #1351

wants to merge 8 commits into from

Conversation

33KK
Copy link

@33KK 33KK commented Oct 25, 2023

  • Download dependencies directly using the DEPS file
  • Don't include any v8 source code in the crate
  • Fix builds for android
  • Move away from relying on secondary_tree to appending rusty_v8 to BUILD.gn

Still TODO is the CI, prebuilts and some other things

Also doesn't use the libicu patch, not sure what it even is for right, crate size concerns, shouldn't be an issue anymore

clangd may or may not work ninja -C target/release/build/v8-****/out/out -t compdb > compile_commands.json

Not tested yet: macos (can't get a vm working), android with external ndk clang, building with deno
Not working yet: windoze with msvc (fails on libicu, EDIT: this actually also fails with upstream rusty_v8)

@CLAassistant
Copy link

CLAassistant commented Oct 25, 2023

CLA assistant check
All committers have signed the CLA.

@33KK
Copy link
Author

33KK commented Oct 25, 2023

What do you even need a CLA for on a MIT licensed repo?

@33KK 33KK force-pushed the main branch 10 times, most recently from fe2fb75 to b5256ab Compare October 26, 2023 19:17
@bartlomieju
Copy link
Member

Thanks for the PR. Could you explain the reason for it? Changing the whole build and CI system is a very invasive change and it would be a lot better to open an issue and discuss it first. The current setup is a result of 4 years of maintenance of this crate and closely follows how V8 is built in Chromium project. I'm wary that we can change it all in one go.

@33KK
Copy link
Author

33KK commented Oct 26, 2023

I started working on this when I tried to get rusty_v8 to build for android. The reason is that the current setup is actually not that close to chromium's build process, which uses gclient. Things need to be forked to use submodules, many of the forks are outdated, some are missing. Some submodule was missing and android build was failing, I decided it was better to just start from scratch rather trying to fix the fundamentally broken setup.

This literally does exactly what using depot_tools would be doing, except without all the bloat and only one dependency of python 3.6+. The DEPS file is parsed and deps are installed and hooks are executed. This doesn't include any v8 code in the crate either, so all the issues with crate size are gone. And it's actually maintainable now, no forks besides main v8 repo are necessary.

There are some issues currently, like the libicu msvc one, but the clang build succeeds. I will try this with the official v8 build procedure later and figure out whats the difference and if there is any in the first place.

@ry
Copy link
Member

ry commented Oct 27, 2023

@33KK This is a big change and we would need to consider it carefully - that said, the build system could definitely be improved. We have been attempting to avoid gclient and instead include all source code in the crate itself, so that no external dependencies needed to be downloaded during the build. I'm not sure how useful that design has been for us. I've often thought that we should refactor rusty_v8 to just depend on gclient and depot_tools and download during the build. It seems you're trying something in between our very manual setup and just depending on gclient. Why not just use gclient? I would very much like to avoid maintaining scripts to parse DEPS.

@33KK
Copy link
Author

33KK commented Oct 27, 2023

The reason is that the core parts of cipd and gclient aren't very complex, depot_tools on the other hand depends on a lot of native binaries and downloads it's own python interpreter, including python2 for some reason; these scripts only really need git and python 3.6+, so they should be much more portable and lightweight.

There really isn't much "parsing" involved, the DEPS file is just valid python, so it's just eval'd. The scripts are very simple really, I never even wrote any python before. The scripts probably could be refactored to be a bit cleaner still.

@ry
Copy link
Member

ry commented Oct 27, 2023

Thanks for the detailed explanation @33KK.

While I can appreciate the lightweight nature of your proposed solution, it does bring in additional points of potential failure and maintenance. Even if the setup is trivial now, we would still be, for example, tethered to the format of the DEPS file. Any changes to this would require us to be vigilant and reactive.

Currently, our manual setup has its quirks, but it's consistent, known, and doesn't introduce dependencies on additional scripts. Our decision to include all source code in the crate, though tedious, was deliberate to avoid external build-time dependencies.

I am not against moving to a more automated approach. But, in my opinion, if we decide to step away from our manual system, we might as well lean into the established infrastructure of gclient. A halfway solution, as you've suggested, may not provide the clarity and simplicity we're aiming for in the long run.

I'm genuinely open to reevaluating our approach, but I believe it's crucial to ensure our chosen path is sustainable, clear, and beneficial in the bigger picture.

@tritao
Copy link

tritao commented Oct 27, 2023

The reason is that the core parts of cipd and gclient aren't very complex, depot_tools on the other hand depends on a lot of native binaries and downloads it's own python interpreter, including python2 for some reason; these scripts only really need git and python 3.6+, so they should be much more portable and lightweight.

I've been testing your scripts and was able to build V8 on Linux with them. There were some minor issues, for which I have a diff with some fixes below. As part of figuring out what was going wrong, I went through the scripts in detail and can confirm they are indeed pretty simple, really a breath of fresh air compared to all the bloat of depot_tools and gclient, great stuff.

diff --git a/scripts/build_v8.py b/scripts/build_v8.py
old mode 100644
new mode 100755
index 11ccfd3..48dec93
--- a/scripts/build_v8.py
+++ b/scripts/build_v8.py
@@ -1,3 +1,5 @@
+#!/usr/bin/env python
+
 import os
 import sys
 import shutil
@@ -107,7 +109,9 @@ def build_v8(
     gn_args["target_cpu"] = q(target_cpu)
     gn_args["v8_target_cpu"] = q(target_cpu)
 
-    gn_args["cc_wrapper"] = q(find_cc_wrapper())
+    cc_wrapper = find_cc_wrapper()
+    if cc_wrapper:
+        gn_args["cc_wrapper"] = q(cc_wrapper)
 
     if not use_custom_libcxx:
         gn_args["use_custom_libcxx"] = False

@33KK
Copy link
Author

33KK commented Oct 28, 2023

The lastchange hook fails to run on the denoland/chromium_build, works fine either without --depth=1, but takes forever to clone, or also works on latest upstream chromium/src/build

@tritao oops, missed that, thanks

@33KK
Copy link
Author

33KK commented Oct 30, 2023

https://groups.google.com/a/chromium.org/g/chromium-dev/c/weYzs3_WWts?pli=1 that's kinda interesting, but doesn't really change much, it still basically requires gclient for the CIPD dependencies, conditions (via gclient-condition in .gitmodules) and hooks; and besides that, it's not even a thing in the v8 repo, at least yet.

The thing I don't like about depot_tools is the complexity and dependence on some third party packages, which basically requires either vpython which downloads 800 MB of stuff or manually setting it up, which may or may not work. On top of gclient would just download everything in the DEPS including the pretty large repos like test262, etc which aren't necessary for the build. Fixing this would require patching the DEPS file, which is an option I guess, but I wouldn't call it a good one.

Another option is just converting DEPS to .gitmodules but that also doesn't really work because of the CIPD dependencies and conditions. Conditions could be hardcoded into the build script though I guess

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.

5 participants