Skip to content

Conversation

@poirierlouis
Copy link
Contributor

Requires:
xmake v2.9.9 to use target option when building with GN tool.

Features:

  • support latest stable version 13.5.212.10
  • tested on Windows only
  • fix syntax to configure "runtimes", only MT for now
  • only build static monolith flavor
  • patch to fix build configuration of a dependency (third_party/partition_alloc/)
  • erase previous version 10.0.58

Notes:

  • add_patches is not used, because afaik, it is not meant to run in this context. gclient is ran during on_install callback. add_patches would try to apply the patch before execution of gclient, that is before files to be patched are even cloned/pulled.

  • see previous v8: fix and update to 13.5.212.10 #6854

Questions:
(1) Is testing on Linux + macOS required to merge?
(2) Is backward compatibility with version 10.0.58 required to merge?

- support latest stable version 13.5.212.10
- tested on Windows only
- fix syntax to configure "runtimes"
- only build static monolith flavor
- patch to fix build configuration of a third party
- erase previous version 10.0.58
@poirierlouis
Copy link
Contributor Author

poirierlouis commented Apr 9, 2025

@waruqi sorry for creating a new PR. Previous one was broken, GitHub couldn't process my last commits somehow.

Follow-up:

#6854 (comment) => please add a sub-directory for version. patches/x.x.x/xxx.patch
#6854 (comment) => please use lower case. and we need not switch directory.
#6854 (comment) => please do not modify user global configuration.

Done

#6854 (comment) => use package:sourcedir()

I tried package:sourcedir() (actually writing the output in a test.log file to see the path). It returns nil. Also I couldn't find a definition for package:sourcedir() in documentation, hence why I used cachedir().
What am I missing?

- don't need to reset core.longpaths option after running "gclient"
- use better API functions to get data from "package"
- fix code style
Untested for macOS, it might be enough.
@poirierlouis
Copy link
Contributor Author

poirierlouis commented Apr 11, 2025

Linux (ubuntu-latest, static, debug):

ar: obj/libv8_monolith.a: error reading obj/v8_base_without_compiler/code.o: No space left on device
ninja: build stopped: subcommand failed.

Looks like CI is not scaled for V8: it does take time/CPU/RAM/SSD. I haven't looked at all logs but I bet most of them failed due to resource exhaustion.

Windows (MSVC 2022, static, x64, MT):

2025-04-11T07:00:45.0227329Z error: patch failed: third_party/partition_alloc/src/partition_alloc/BUILD.gn:41
2025-04-11T07:00:45.0228022Z error: third_party/partition_alloc/src/partition_alloc/BUILD.gn: patch does not apply
2025-04-11T07:00:45.0257025Z �[0m�[2;31;1merror: �[0m@programdir\core\sandbox\modules\os.lua:378: execv(git apply D:\a\xmake-repo\xmake-repo\packages\v\v8\patches\13.5.212.10\fix.patch) failed(1)

Builds passed for versions 13.3.415.19 and 13.4.114.19.
It fails for 13.5.212.10 when trying to apply the patch. Which I'm not sure why yet. I'll take a look.

Is it required to pass all CI tests?
Can it be limited to only static / release / MT flags, and MSVC 2022? And later increase compatibility range...

@waruqi
Copy link
Member

waruqi commented Apr 11, 2025

Looks like CI is not scaled for V8: it does take time/CPU/RAM/SSD. I haven't looked at all logs but I bet most of them failed due to resource exhaustion.

If we cannot confirm that the package works properly due to CI resource limits.

One solution is to improve CI and try to increase the resource limits. If that is not possible, at least the contributors need to confirm that the tests pass on their local machines.

@poirierlouis
Copy link
Contributor Author

poirierlouis commented Apr 11, 2025

Is it possible to add some kind of CI flags in xmake scripts/test.lua?
Regarding disk space on Windows, max is set to 32GB. It currently builds 2 versions, but the last one don't have enough space.
Instead of increasing max size in CI, what about changing test.lua: such as when multiple versions are tested, it will clean cache folder one after the other (free storage space)?

e.g.

  • test v1 => fail or pass => clean cache
  • test v2 => fail or pass => clean cache
  • ...

This way no need to increase storage space in CI, and it would be better in the long term anyway. The amount of versions will eventually be too high to request an even more growing storage space on CI side.

I can build in both environments locally (linux + windows). Is it possible to restrict compatibility of a package?

@waruqi
Copy link
Member

waruqi commented Apr 15, 2025

The default is to install them in parallel. This can greatly improve the efficiency of test installation for most small libraries. However, we cannot distinguish some large libraries and implement serial compilation. I think you can solve this problem by submitting one version at a time instead of modifying multiple versions at once.

@waruqi
Copy link
Member

waruqi commented Apr 15, 2025

I removed other two versions, if it works on ci, you can open two new prs to add left two versions.

- configure a dummy Git account, required to use 'gclient'
- remove patch, build issue seems to have been fixed by Google.
@poirierlouis
Copy link
Contributor Author

Thanks, I was going to remove them eventually. Better start with the latest version for now anyway.

I fixed two issues this time:

  • Git account required (email + name) to build on Linux
  • patch is obsolete (is not applied), build issue seems to have been fixed by Google

@poirierlouis
Copy link
Contributor Author

Much better. Windows 2019 is not supported afaik for V8 anyway.

Linux storage space issue though. Can it be configured to increase storage space in CI?

@waruqi
Copy link
Member

waruqi commented Apr 16, 2025

Much better. Windows 2019 is not supported afaik for V8 anyway.

you can add on_check to disable it.

Linux storage space issue though. Can it be configured to increase storage space in CI?

You can increase it in this pr if it can.

@poirierlouis
Copy link
Contributor Author

you can add on_check to disable it.

Thanks, I added some checks and restrictions (Linux / Windows / static / release).

You can increase it in this pr if it can.

I'm using an action to free disk space (common technique for free/public plans). We'll see if it is enough, and hopefully don't break things.

@poirierlouis
Copy link
Contributor Author

I'll see if I can fix builds for Archlinux, Fedora and Linux arm64 (locally first).
I will also add a check regarding runtime, it only supports MT for now.

Ignore MD compilation; it would need tests to actually make V8 compliant with MD, if possible.
Should fix errors of compilation due to missing headers.
@poirierlouis
Copy link
Contributor Author

Attempt to fix Archlinux and Fedora.
It should also ignore compilation for MD runtime.

I'm wondering if the added action for ubuntu workflow can cause issues for other packages?

@poirierlouis
Copy link
Contributor Author

Sorry check for MT was too global instead of scoped for Windows.

@poirierlouis
Copy link
Contributor Author

poirierlouis commented Apr 21, 2025

Alright, so I removed the redundant check, only readonly runtimes is used.
I also reverted changes regarding Archlinux / Fedora, I couldn't fix these builds. So not supporting them for now.

Afaik, it is fine now. Let me know if something needs to be changed before merging.

@waruqi
Copy link
Member

waruqi commented Apr 24, 2025

Alright, so I removed the redundant check, only readonly runtimes is used. I also reverted changes regarding Archlinux / Fedora, I couldn't fix these builds. So not supporting them for now.

Afaik, it is fine now. Let me know if something needs to be changed before merging.

nothing, but please fix left ci errors.

-- Only configured and tested for:
assert(not package:is_debug() and not package:config("shared"), "package(v8): only configured for static + release usage")
assert(not package:is_debug() and not package:config("shared"), "package(v8): only configured for static + release usage.")
assert(not is_host("arch"), "package(v8): Archlinux is not supported.")
Copy link
Member

Choose a reason for hiding this comment

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

arch? no this name, only linux.

and why use is_host? v8 is library, we should use package:is_plat

Copy link
Contributor Author

@poirierlouis poirierlouis Apr 24, 2025

Choose a reason for hiding this comment

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

I'll change to use package:is_plat everywhere instead of is_host then.
But I don't understand about "only linux". How can I deduce whether it is Archlinux / Fedora to only disable them, to fix CI errors?

Copy link
Member

Choose a reason for hiding this comment

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

use linuxos.name

https://github.com/xmake-io/xmake/blob/386725082bbea11b0fb3894c7b3ae7411b375ae0/xmake/core/base/linuxos.lua#L80

is_host("linux") + linuxos.name()

BTW, Why are libraries still bundled with distributions? We should fix them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I changed it.

Why are libraries still bundled with distributions? We should fix them.

What do you mean? That building V8 for Archlinux / Fedora should be supported and that it should work?

@waruqi waruqi merged commit 2371706 into xmake-io:dev Apr 25, 2025
73 checks passed
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.

2 participants