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

Manually version browsers #74

Closed
5 of 7 tasks
dgp1130 opened this issue Jul 25, 2023 · 1 comment
Closed
5 of 7 tasks

Manually version browsers #74

dgp1130 opened this issue Jul 25, 2023 · 1 comment
Labels
cleanup Internal cleanup of infrastructure
Milestone

Comments

@dgp1130
Copy link
Owner

dgp1130 commented Jul 25, 2023

@io_bazel_rules_webtesting doesn't appear to be maintained anymore and hasn't see a release in over 2 years. This means the version of downloaded browsers is falling out of date.

It wouldn't be that big a deal, except that I've been using <template shadowroot="open"> because that is what's supported in the version of Chrome used for testing. However, the spec was changed to use <template shadowrootmode="open"> and Chrome M119 is slated to remove support for <template shadowroot="open">. This is currently scheduled for Nov 7th, per https://chromiumdash.appspot.com/schedule.

As a result, I need to update my code to use shadowrootmode, which means I need a recent-enough browser version which supports that. I'm wondering if I can fork or build a simpler solution good enough for @rules_prerender. This is definitely blocking 1.0.0 because I wouldn't want to ship that until I can use shadowrootmode correctly.

I have a prototype in ref/browser-update forked from an implementation in the Angular Dev Infra repository. This was sufficient to download recent versions of Chrome / Firefox and get a passing test with shadowrootmode on Chrome.

Some more things I'll need to figure out:

  • Firefox - Doesn't seem to be starting correctly in WSL right now. I would love to do better cross-browser testing, but Firefox doesn't support declarative shadow DOM right now, which is a big part of @rules_prerender. It might be fine since I have the polyfill, and could serve as a test case for that. But we may want to think about how much value this is actually giving and whether or not its worth it.
  • Debugging - I'm struggling to start headful (?) Chrome to debug a test like I've done in the past. Might take some trial and error to figure that out.
  • Test everything - New Chrome browser should be enabled everywhere in the repository.
  • Updating browsers - There's a script in Angular Dev Infra for updating browser version automatically which would be useful to fork as well.
  • Refactor - I kept the existing directory structure and formatting conventions which definitely clashes with @rules_prerender. I'll want to refactor things to make it more maintainable and aligned with the rest of the codebase.
  • Migrate to shadowrootmode - Once everything is stable, I should be able to migrate all usages of shadowroot to shadowrootmode.
  • Break @io_bazel_rules_webtesting dependency - If possible, I should also look into breaking the dependency on @io_bazel_rules_webtesting entirely. Unfortunately the Angular implementation does still rely on a few exported symbols from that ruleset, even if it versions its own browsers. It would be great to drop that dependency altogether.
@dgp1130 dgp1130 added the cleanup Internal cleanup of infrastructure label Jul 25, 2023
@dgp1130 dgp1130 added this to the 1.0.0 milestone Jul 25, 2023
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

https://github.com/angular/dev-infra/tree/1ad20ef9dd457de967252283c1a968b0d702d0ae/bazel/browsers/

This is the only existing example I could find which supports a setup for manually versioning browsers in the repository. It still has a dependency on `@io_bazel_rules_webtesting`, but hopefully we can break that eventually.

This commit is a straight copy with *zero* modifications. The only difference is adding a `README` to describe where the fork came from. Any required changes will come in future commits.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

We _must_ drop the `@io_bazel_rules_webtesting` `browser_repositories` call because it will download to the same Bazel repository names and conflict with the locally versioned browsers.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

This is necessary for `web_test` to correctly run the tests. Both Chromium and Firefox seem to work with native.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

This updates the Chromium browser to choose a forked configuration without `--headless` when in debug mode. This way the browser can be inspected during tests.

For some reason, `--//tools/browsers:use_debug_browers` needed to be enabled manually. I thought this enabled by `--config debug`, but apparently not. I've added it to `debug.bazelrc` to make this more automatic.
dgp1130 added a commit that referenced this issue Jul 29, 2023
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

This updates the WebDriver library to dynamically read the current browser from the environment and then set up WebDriver for that browser. This makes it work with Chromium or Firefox without hard-coding either browser.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

Implemented as just an alias for now, will refactor afterwards.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

This is to align with the version used in Angular Dev Infra's browser update tool. Also it's just a good upgrade to do.

The API changed slightly in Yargs, and we need to explicitly stay synchronous with `.parseSync`. I took the opportunity to standardize some of the formatting on the various tools. Also adds `.strict()` which I just discovered and which ensures no unknown commands are passed.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

This is a straight copy of the relevant files with no modifications made to the files to be clear about changes in subsequent diffs. The only difference is that the files are named `*.mts` instead of `*.ts` to match existing repository conventions.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

This migrates the build process to an `@aspect_rules_js`-based toolchain and installs the required dependencies. I opted to just remove the `Spinner` class usage in order to break a dependency on Angular Dev Infra utils I didn't want to fork.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

A decent portion of this script was dedicated to pushing the browser binaries to a mirror owned by Angular. We don't have permission to do that here so it can be dropped.

I'm a little sad to drop the Firefox part, as it seems only Chrome is supported for pulling actual version numbers. I may try to revive that in the future if and when upgrading Firefox becomes an issue.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

We don't want this dependency on Angular. Also removes a comment about Firefox download instructions, no such instructions exist either here or at the location we forked from.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

This aligns conventions with the rest of the repo, but does not make any behavioral changes.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

Hopefully I didn't break anything, these are fairly trivial changes.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

Also updates `findLatestRevisionForAllPlatforms` to return an exit code, which I think is a little cleaner than the `process.exit` call.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

This makes things more cacheable and explicit. I needed to break a circular dependency between `Browser` and `BrowserArtifact` to make this work. Also renamed the files to use `_` in the name to align with repository conventions.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

This was likely used for publishing the content to NPM, but that's not a requirement here.
dgp1130 added a commit that referenced this issue Jul 29, 2023
dgp1130 added a commit that referenced this issue Jul 29, 2023
…code.

Refs #74.

This brings it in line with repository conventions.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

Modifications were simply a find and replace to update `//bazel/constraints` -> `//tools/constraints`. No manual edits appear to be needed.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

Modifications were just a find-and-replace of `//bazel/browsers` -> `//tools/browsers`. No manual modifications appear to be needed.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

This reduces boilerplate and gives a set of "default" browsers to use for such tests.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

For whatever reason, `web_test_suite` *requires* the target name to be explicit. `//foo:foo` is required instead of `//foo`. This appears to be an overly constrained string parsing behavior. To make this more usable, `jasmine_web_test_suite` now automatically expands any input targets from `//foo` to `//foo:foo` if necessary before passing them in to `web_test_suite`.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

These were needed for browsers versioned by `@io_bazel_rules_webtesting` but now are never used. We only need tags for the locally versioned browser instances.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

All usages have been migrated to `//tools/browsers/chromium`, so this is no longer needed.
dgp1130 added a commit that referenced this issue Jul 29, 2023
Refs #74.

Chrome originally shipped declarative shadow DOM with `shadowroot` as the relevant attribute name, however the spec changed to `shadowroot` and Chrome has deprecated the old behavior which will be removed soon. Therefore we should switch to `shadowrootmode` to align with the spec and support multiple browsers. See https://chromestatus.com/feature/6239658726391808.

This also might enable streaming support for DSD in Chrome? https://chromestatus.com/feature/5161240576393216

This migration was mostly done via a find and replace with some manual sanity checks.
dgp1130 added a commit that referenced this issue Jul 30, 2023
Refs #74.

Unfortunately hardware accelleration seems to be crashing Firefox on GitHub actions.
@dgp1130
Copy link
Owner Author

dgp1130 commented Jul 30, 2023

I've made a lot of progress here and landed locally versioned browsers. Chromium and Firefox are versioned locally and run for all e2e tests by default successfully. I then was able to update <template shadowroot="open"> -> <template shadowrootmode="open"> to fix the main issue. However there are two tasks I've failed at so far.

First, Firefox works great locally, but I have yet to get it to successfully run in CI. I'm not entirely sure what's going on as I've whack-a-moled a few errors. The one I'm at now is:

[GFX1-]: glxtest: Unable to open a connection to the X server
[GFX1-]: glxtest: libEGL initialize failed
[GFX1-]: RenderCompositorSWGL failed mapping default framebuffer, no dt

I don't fully understand what this means, but libegl doesn't seem to be configured correctly. I tried installing it through various means with no luck. eglinfo gives no so helpful output:

NEEDRESTART-VER: 3.5
NEEDRESTART-KCUR: 5.15.0-[104](https://github.com/dgp1130/rules_prerender/actions/runs/5703331116/job/15455875219#step:3:105)1-azure
NEEDRESTART-KEXP: 5.15.0-1041-azure
NEEDRESTART-KSTA: 1
libEGL warning: failed to open /dev/dri/card0: Permission denied
EGL client extensions string:
    EGL_EXT_device_base EGL_EXT_device_enumeration EGL_EXT_device_query
    EGL_EXT_platform_base EGL_KHR_client_get_all_proc_addresses
    EGL_EXT_client_extensions EGL_KHR_debug EGL_EXT_platform_device
    EGL_EXT_platform_wayland EGL_KHR_platform_wayland
    EGL_EXT_platform_x11 EGL_KHR_platform_x11 EGL_EXT_platform_xcb
    EGL_MESA_platform_gbm EGL_KHR_platform_gbm
    EGL_MESA_platform_surfaceless


GBM platform:
eglinfo: eglInitialize failed

Wayland platform:
eglinfo: eglInitialize failed

X11 platform:
eglinfo: eglInitialize failed

libEGL warning: failed to open /dev/dri/card0: Permission denied
Device platform:
eglinfo: eglInitialize failed

My takeaway is that it's failing to initialize, but I have no idea why or what to do to fix it. Resources on this are unfortunately pretty scarce. libegl seems to be related to 3D hardware acceleration, so I looked into turning that off altogether for Firefox. This appears to be possible, but only in the settings UI. I couldn't find a flag to do it.

For now, I have Firefox tests running with the noci tag. That at least means I'll see test failures locally with bazel test //..., even if they don't show in CI. My current best attempt is in the firefox branch, but I think I need to take a break on this for a while and come back later. I filed #75 to follow up on this at some point.

Second I wasn't able to break the dependency on @io_bazel_rules_webtesting. I was hoping this wouldn't be too bad, but there is a significant amount of code there doing real work I can't drop. I initially forked the repo into @rules_prerender, linked it locally with local_repository and then started deleting everything I could. I was hoping it was just build rules, but the repository has a non-trivial amount of Go infrastructure it needs to even start a web_test_suite test. I don't see a local fork as an advantage over an unmaintained dependency in this case, so I'd rather just leave the dep as is. It's a bit unfortunate, but it's the least bad option right now. My attempt is in ref/rm-rules-webtesting, but I don't think I'll be moving forward with it.

For now, I'll close this issue and follow up on the Firefox thing separately.

@dgp1130 dgp1130 closed this as completed Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Internal cleanup of infrastructure
Projects
None yet
Development

No branches or pull requests

1 participant