Reapply "devops(docker): split browser layers and use zstd compressio…#41232
Reapply "devops(docker): split browser layers and use zstd compressio…#41232KRRT7 wants to merge 10 commits into
Conversation
…n for faster pulls (microsoft#40702)" (microsoft#41081) This reverts commit 67930e8.
|
@dgozman ^^ |
|
@dcrousso I tagged them so that they'd trigger the GHAs, I have no idea what you guys saw, I've been using it on my linux / macOS machines with no issues, I'd need to trigger the issue for me to be able to debug edit: sorry, could've been clearer about my intentions |
|
as per #41081 (comment) without the logs, best I can hope for is for it to get triggered in your CI for me to be able to analyze. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
hmm, ok, I think I know what's up. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@KRRT7 Seems to be still failing. |
|
@dgozman sorry for the delay, should be good now, I tried it on my MacOS and via dockers and now everything passes for the issues I found |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| rm -rf ~/.npm/ && \ | ||
| chmod -R 777 /ms-playwright | ||
| # Keep the full browser registry writable for later npm ci runs, including .links. | ||
| chmod -R a+rwx /ms-playwright && \ |
There was a problem hiding this comment.
Won't this touch the whole /ms-playwright, including previously installed browsers, and make the last layer very large? Perhaps move this to the first layer instead?
| @@ -11,7 +11,6 @@ ENV LC_ALL=C.UTF-8 | |||
| # === INSTALL Node.js === | |||
There was a problem hiding this comment.
Note we also have Dockerfile.resolute now.
Test results for "MCP"1 failed 7340 passed, 1122 skipped Merge workflow run. |
Test results for "tests 1"1 flaky39584 passed, 743 skipped Merge workflow run. |
|
geez, dunno why it fails on there but not locally, let me take another look |
|
I refactored the shared browser install logic into utils/docker/install_browsers.sh so jammy/noble/resolute stay in sync going forward while preserving separate Docker RUN layers for chromium/firefox/webkit. |
dgozman
left a comment
dgozman
left a comment
There was a problem hiding this comment.
I think we went backwards, from an almost-working solution to a larger refactoring that is less likely to land.
| @@ -0,0 +1,47 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
I am not entirely convinced this separate script makes it easier to maintain things. Also, we are now mixing up a refactoring and a "split layers" feature, which makes it hard to review what exactly is changing in the Dockerfile.
There was a problem hiding this comment.
you still get the split layers feature, whilst consolidating duplicate logic in 3 files, which I faced an issue with because I kept changing things in jammy, and forgetting to update noble / resolute.
This reverts commit 59098d2.
3 participants
…n for faster pulls (#40702)" (#41081)
This reverts commit 67930e8.