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

Upgrade the version of angle #69

Merged
merged 19 commits into from
Dec 6, 2023
Merged

Upgrade the version of angle #69

merged 19 commits into from
Dec 6, 2023

Conversation

sagudev
Copy link
Member

@sagudev sagudev commented Oct 13, 2023

Updates angle to 02755361e26d82768eb1d5f576145e19d7c265cd

LibANGLE is not separate target anymore, but now we compile all gfx/angle/targets as separate libs (to be able to reuse as much as possible and avoid recompilation) and then link them into final targets (translator, libGLES, libEGL). We also need zlib (but only on egl feature).

Due to amount of changes (some of them also in public api), we need to do version bump (already included in PR).

Given how big this change is it would be good to test it on all consumers:

@sagudev sagudev force-pushed the bump branch 5 times, most recently from e29b44b to 73e8825 Compare October 13, 2023 11:26
@sagudev sagudev mentioned this pull request Oct 14, 2023
3 tasks
@sagudev
Copy link
Member Author

sagudev commented Oct 15, 2023

After reviewing https://bugzilla.mozilla.org/show_bug.cgi?id=1578576 I think only libGLES needs to depend on libANGLE stuff as libEGL depends on libGLES anyway.

@sagudev sagudev force-pushed the bump branch 2 times, most recently from 9ee0862 to 3bba01a Compare October 15, 2023 16:01
@sagudev

This comment was marked as outdated.

@mrobinson
Copy link
Member

I believe the change described in #33 was done in #58, but it seems like currently this PR undoes it?

@sagudev
Copy link
Member Author

sagudev commented Oct 17, 2023

We do not have libANGLE target anymore. But this commits are just testing/experimenting, currently I am working to build every gfx/angle/target as separate lib, so we can reuse even more.

@sagudev sagudev force-pushed the bump branch 2 times, most recently from 5569a02 to c97c65a Compare October 17, 2023 13:59
@sagudev sagudev force-pushed the bump branch 2 times, most recently from c152b1c to 68ed969 Compare October 18, 2023 11:28
@sagudev
Copy link
Member Author

sagudev commented Oct 18, 2023

I am currently working on wr companion, linking works now, but some egl commands fail.

@sagudev sagudev marked this pull request as ready for review October 26, 2023 04:56
@sagudev
Copy link
Member Author

sagudev commented Oct 26, 2023

I think this is ready for review! Companions PR are working so there shouldn't be any problems.

@sagudev sagudev requested review from jdm and mrobinson October 26, 2023 04:59
@sagudev sagudev changed the title Bump angle to 02755361e26d82768eb1d5f576145e19d7c265cd Bump angle Oct 26, 2023
Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Nice work. I have a few comments, but this looks great in general.

build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
return;
}

println!("build_lib: {lib:?}");
Copy link
Member

Choose a reason for hiding this comment

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

I think this was left by accident?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really as this is only printed if build script panics when executing, so it will help debugging.

build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Show resolved Hide resolved
@@ -293,15 +366,11 @@ fn fixup_path(path: &str) -> String {
}

#[cfg(feature = "egl")]
fn generate_bindings() {
fn generate_gl_bindings() {
println!("generate_gl_bindings");
Copy link
Member

Choose a reason for hiding this comment

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

Another stray println?

build.rs Outdated
Comment on lines 40 to 42
for lib in build_data::GLESv2.use_libs {
build_lib(&mut libs, &target, *lib);
}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of use_libs here I think a more standard name might be library_dependencies.

Copy link
Member Author

@sagudev sagudev Oct 26, 2023

Choose a reason for hiding this comment

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

This field is named after option from moz.build files.

src/tests.rs Outdated Show resolved Hide resolved
patches/new-compilers.patch Outdated Show resolved Hide resolved
@sagudev sagudev requested a review from mrobinson October 27, 2023 05:20
Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Looks good, but with some changes below before landing.

I have one more question about build-time. Looking at the actions in GitHub it seems the build is now several minutes slower. Any idea what is causing that?

build.rs Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated
.flag_if_supported("-msse2") // GNU
.flag_if_supported("-arch:SSE2"); // MSVC
if matches!(lib, Libs::ANGLE_COMMON) {
// Hard-code lines like `if CONFIG['OS_ARCH'] == 'Darwin':` in moz.build files
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this comment clearer then since we are modifying the code.

build.rs Outdated Show resolved Hide resolved
generate_build_data.py Outdated Show resolved Hide resolved
@sagudev
Copy link
Member Author

sagudev commented Oct 30, 2023

Looking at the actions in GitHub it seems the build is now several minutes slower. Any idea what is causing that?

Actually, first runs when I refactored build.rs to what it is now, build times were better than on master, but next few runs do not provide such great timings (newer runs contained only small fixups). As noted in servo Windows runners have varying performances.

Also given that new angle has a lot more code (the most important change being support for OpenGL), some changes in build time are expected (when WIP used minimal changed build.rs build times were 30min).

I do plan to investigate sccache support for followup, so we should get better build times.

@mrobinson
Copy link
Member

Looking at the actions in GitHub it seems the build is now several minutes slower. Any idea what is causing that?

Actually, first runs when I refactored build.rs to what it is now, build times were better than on master, but next few runs do not provide such great timings (newer runs contained only small fixups). As noted in servo Windows runners have varying performances.

Also given that new angle has a lot more code (the most important change being support for OpenGL), some changes in build time are expected (when WIP used minimal changed build.rs build times were 30min).

I do plan to investigate sccache support for followup, so we should get better build times.

Great. Thanks for the information.

@sagudev
Copy link
Member Author

sagudev commented Oct 30, 2023

I would still like to wait for review from @jdm given the size of PR.

@sagudev
Copy link
Member Author

sagudev commented Nov 25, 2023

jdm isn't able to review it, therefore we should just land it.

generate_build_data.py Outdated Show resolved Hide resolved
@mrobinson mrobinson changed the title Bump angle Upgrade the version of angle Dec 6, 2023
@mrobinson mrobinson enabled auto-merge December 6, 2023 09:46
@mrobinson mrobinson added this pull request to the merge queue Dec 6, 2023
Merged via the queue into servo:main with commit be871c1 Dec 6, 2023
7 checks passed
@sagudev sagudev mentioned this pull request Dec 7, 2023
2 tasks
@sagudev sagudev mentioned this pull request Dec 23, 2023
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