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

Link ICU data into the Cobalt binary #3211

Merged
merged 7 commits into from
Jun 7, 2024

Conversation

hlwarriner
Copy link
Contributor

@hlwarriner hlwarriner commented May 9, 2024

With this change, Cobalt will no longer be deployed with an external ICU data file that must be loaded at runtime.

b/209049814

Change-Id: I8f67b3176181fa314f3e011ce4cf07fd4a671436

Test-On-Device: true

@hlwarriner
Copy link
Contributor Author

This is another attempt at #838 and attempts to make the change for all architectures and platforms.

I'll want to do some manual testing on devices before merging this but wanted to share it now to get feedback. There are a few platforms I haven't yet tried to build for with these changes (e.g., Windows platforms), and I expect more work may be needed.

@hlwarriner
Copy link
Contributor Author

This is another attempt at #838 and attempts to make the change for all architectures and platforms.

I'll want to do some manual testing on devices before merging this but wanted to share it now to get feedback. There are a few platforms I haven't yet tried to build for with these changes (e.g., Windows platforms), and I expect more work may be needed.

Yeah, I already see a build failure related to the assembly file on xbox. I'll look into it.

third_party/icu/scripts/make_data_assembly.py Show resolved Hide resolved
third_party/icu/BUILD.gn Outdated Show resolved Hide resolved
third_party/icu/README.cobalt Outdated Show resolved Hide resolved
@oxve
Copy link
Contributor

oxve commented May 13, 2024

Looks like this ran into a similar issue as b/213960038: "compiler is out of heap space"

I suspect that the inclusion of the dat file could run into some limit on win32 builds (only xb1 and win32 failed). Have you been able to build this locally on win32?

@hlwarriner
Copy link
Contributor Author

Looks like this ran into a similar issue as b/213960038: "compiler is out of heap space"

I suspect that the inclusion of the dat file could run into some limit on win32 builds (only xb1 and win32 failed). Have you been able to build this locally on win32?

Looks like this ran into a similar issue as b/213960038: "compiler is out of heap space"

I suspect that the inclusion of the dat file could run into some limit on win32 builds (only xb1 and win32 failed). Have you been able to build this locally on win32?

Thanks, yeah, we did build locally (thanks @andrewsavage1) and ran into different issues because the generated inline assembly was incompatible with MSVC. I'm setting up a Windows machine now and will be focusing on this.

Copy link
Contributor

@y4vor y4vor left a comment

Choose a reason for hiding this comment

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

LGTM on my side

@hlwarriner
Copy link
Contributor Author

Looks like this ran into a similar issue as b/213960038: "compiler is out of heap space"
I suspect that the inclusion of the dat file could run into some limit on win32 builds (only xb1 and win32 failed). Have you been able to build this locally on win32?

Looks like this ran into a similar issue as b/213960038: "compiler is out of heap space"
I suspect that the inclusion of the dat file could run into some limit on win32 builds (only xb1 and win32 failed). Have you been able to build this locally on win32?

Thanks, yeah, we did build locally (thanks @andrewsavage1) and ran into different issues because the generated inline assembly was incompatible with MSVC. I'm setting up a Windows machine now and will be focusing on this.

It took a few days but my Windows machine is now set up to a point where I can start building Cobalt and working on a fix. If I don't make progress in the next couple days, though, I'll likely modify this PR to only make the ICU data change for Evergreen for now. The time pressure exists only for Evergreen, and it looks like it wouldn't be too bad to have the build code branch on Evergreen for this behavior.

@y4vor @kaidokert FYI.

@hlwarriner hlwarriner marked this pull request as draft May 29, 2024 23:37
@hlwarriner
Copy link
Contributor Author

Looks like this ran into a similar issue as b/213960038: "compiler is out of heap space"
I suspect that the inclusion of the dat file could run into some limit on win32 builds (only xb1 and win32 failed). Have you been able to build this locally on win32?

Looks like this ran into a similar issue as b/213960038: "compiler is out of heap space"
I suspect that the inclusion of the dat file could run into some limit on win32 builds (only xb1 and win32 failed). Have you been able to build this locally on win32?

Thanks, yeah, we did build locally (thanks @andrewsavage1) and ran into different issues because the generated inline assembly was incompatible with MSVC. I'm setting up a Windows machine now and will be focusing on this.

It took a few days but my Windows machine is now set up to a point where I can start building Cobalt and working on a fix. If I don't make progress in the next couple days, though, I'll likely modify this PR to only make the ICU data change for Evergreen for now. The time pressure exists only for Evergreen, and it looks like it wouldn't be too bad to have the build code branch on Evergreen for this behavior.

@y4vor @kaidokert FYI.

We discussed offline and do have a plan for win32: we'll skip the inline assembly and use NASM to assemble the generated data assembly. I converted this to a draft for now but should have a chance to upload a new commit early next week.

@hlwarriner hlwarriner removed the request for review from johnxwork June 3, 2024 17:55
@hlwarriner hlwarriner marked this pull request as ready for review June 3, 2024 18:52
@hlwarriner hlwarriner requested a review from kaidokert June 3, 2024 18:52
@hlwarriner
Copy link
Contributor Author

Looks like this ran into a similar issue as b/213960038: "compiler is out of heap space"

I suspect that the inclusion of the dat file could run into some limit on win32 builds (only xb1 and win32 failed). Have you been able to build this locally on win32?

This seems to be resolved now; I'm guessing it was related to the inline data assembly file, which we're no longer generating.

@hlwarriner
Copy link
Contributor Author

Looks like this ran into a similar issue as b/213960038: "compiler is out of heap space"
I suspect that the inclusion of the dat file could run into some limit on win32 builds (only xb1 and win32 failed). Have you been able to build this locally on win32?

Looks like this ran into a similar issue as b/213960038: "compiler is out of heap space"
I suspect that the inclusion of the dat file could run into some limit on win32 builds (only xb1 and win32 failed). Have you been able to build this locally on win32?

Thanks, yeah, we did build locally (thanks @andrewsavage1) and ran into different issues because the generated inline assembly was incompatible with MSVC. I'm setting up a Windows machine now and will be focusing on this.

It took a few days but my Windows machine is now set up to a point where I can start building Cobalt and working on a fix. If I don't make progress in the next couple days, though, I'll likely modify this PR to only make the ICU data change for Evergreen for now. The time pressure exists only for Evergreen, and it looks like it wouldn't be too bad to have the build code branch on Evergreen for this behavior.
@y4vor @kaidokert FYI.

We discussed offline and do have a plan for win32: we'll skip the inline assembly and use NASM to assemble the generated data assembly. I converted this to a draft for now but should have a chance to upload a new commit early next week.

Ok, this is ready for review again. Thanks for the discussion and ideas around this, @andrewsavage1 @kaidokert @osagie98.

Copy link
Contributor

@andrewsavage1 andrewsavage1 left a comment

Choose a reason for hiding this comment

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

As long as this is working on all platforms now it lgtm!

third_party/icu/scripts/make_data_assembly.py Show resolved Hide resolved
third_party/icu/scripts/make_data_assembly.py Outdated Show resolved Hide resolved
third_party/icu/scripts/make_data_assembly.py Show resolved Hide resolved
Copy link
Member

@kaidokert kaidokert left a comment

Choose a reason for hiding this comment

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

LGTM - it's great that a lot of GN deps get removed.

Please make sure that internal builds also run

With this change, Cobalt will no longer be deployed with an external ICU
data file that must be loaded at runtime.

b/209049814

Change-Id: I8f67b3176181fa314f3e011ce4cf07fd4a671436
Change-Id: I6a168818d31285c9c43b6e6d58128c48066078e8
Change-Id: Ia906aab5f249c8eb9f4b99f371e2e8f3a6802840
Change-Id: Ibd723905104aa0c283d7f5eed4f7306612d0bc9d
MSVC doesn't support inline data assembly for win32, so we'll skip that
extra step that's taken in upstream. Instead, we use NASM to assemble
the generated data assembly.

Change-Id: Ibaa542b3ee2d3bba2eacf3bedbc3021793910b33
Change-Id: I31a7020f4746f21f156e2543751212d753159700
Change-Id: I9433f450c885db2389e21ffd0601ae43009a672d
@hlwarriner hlwarriner requested a review from a team as a code owner June 6, 2024 15:38
@hlwarriner
Copy link
Contributor Author

LGTM - it's great that a lot of GN deps get removed.

Please make sure that internal builds also run

The internal builds are now all succeeding. There are unrelated issues with the internal tests, so I'm planning to just go ahead and merge this today.

@hlwarriner hlwarriner merged commit 59d8eaf into youtube:main Jun 7, 2024
679 of 687 checks passed
@hlwarriner hlwarriner added the cp-25.lts.1+ Cherry Pick to the 25.lts.1+ branch label Jun 7, 2024
cobalt-github-releaser-bot pushed a commit that referenced this pull request Jun 7, 2024
With this change, Cobalt will no longer be deployed with an external ICU
data file that must be loaded at runtime.

b/209049814

Change-Id: I8f67b3176181fa314f3e011ce4cf07fd4a671436

Test-On-Device: true
(cherry picked from commit 59d8eaf)
hlwarriner added a commit that referenced this pull request Jun 8, 2024
Refer to the original PR: #3211

With this change, Cobalt will no longer be deployed with an external ICU
data file that must be loaded at runtime.

b/209049814

Change-Id: I8f67b3176181fa314f3e011ce4cf07fd4a671436

Test-On-Device: true

Co-authored-by: Holden Warriner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cp-25.lts.1+ Cherry Pick to the 25.lts.1+ branch on_device
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants