Skip to content

Conversation

@joperezr
Copy link
Member

cc: @mthalman @akoeplinger

On the previous PR we were installing v8 engine from npm and jsvu whcih had the potential issue of failing to install npm which was being installed from a external script. This will switch to instead install it from blob storage and mimic the same way we do it for our helix queues, with the added benefit that engines won't be installed under /root/.jsvu meaning v8 will now be able to be ran by our trimming tests which is not working today.

@mthalman
Copy link
Member

It'd be nice if we could avoid defining a custom script file here, particularly one which has a dependency on Arcade's native toolset backend. Is there a way that the https://github.com/dotnet/arcade/blob/master/eng/common/init-tools-native.sh file, or some other Arcade file, could be used to explicitly install a known tool rather than the script relying on a global.json file? Could that script be refactored to enable explicit installation in this manner? cc @riarenas

@akoeplinger
Copy link
Member

I think we could also pull directly from https://storage.googleapis.com/chromium-v8/official/canary/v8-linux64-rel-8.5.183.zip instead of the arcade blob storage.

The reason why we didn't do that in the Helix script was to avoid hitting Google's servers every time a new Helix machine is provisioned. Since the Docker image is rebuilt only once in a while this shouldn't be an issue here.

We could also inline the linux-v8-engine.sh into the Dockerfile and remove the argument validation (we'll never call it with wrong values anyway) to shrink it.

@joperezr
Copy link
Member Author

So I opted instead to inline the parts of the script that mattered (removed all parameter validation). I'm still not downloading it from Google's servers as @akoeplinger suggested above, since these will still be built every CI run and we wouldn't want to break CI if google servers are down.

Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

LGTM!

@joperezr
Copy link
Member Author

Merging to unblock dotnet/runtime#48429

@joperezr joperezr merged commit 3852446 into dotnet:master Feb 19, 2021
@joperezr joperezr deleted the InstallFromBlob branch February 19, 2021 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants