posit-dev / positron

Positron, a next-generation data science IDE
Other
2.37k stars 69 forks source link

Builds: Compile built-in extensions only once (improve build speed & upstream alignment) #965

Open jmcphers opened 1 year ago

jmcphers commented 1 year ago

Currently, the Positron build scripts create a universal macOS binary by doing a full build of arm64 and a full build of x86, and then stitching them together.

This sort of works, but it requires some hacks. For example, some of the build artifacts aren't compatible with this approach because the process that produces them is not stable. In order for us to stitch together the two independent builds using create-universal-app, every file must match, or support being fused. This is the reason for this code, which, while not outright smelly, does at least have a very unappetizing aroma.

https://github.com/rstudio/positron/blob/c2d0ec0974d45eaa249515234bae4ee484138d66/.github/workflows/build-release-macos.yml#L201-L225

This isn't the way that VS Code works; its core TypeScript compilation step happens only once. The compilation results, which aren't architecture-specific, are cached and reused for the arm64 build, the x86 build, and the Universal build. Here is where the compilation artifact is downloaded:

https://github.com/rstudio/positron/blob/c2d0ec0974d45eaa249515234bae4ee484138d66/build/azure-pipelines/darwin/product-build-darwin.yml#L33-L40

We cannot do this in Positron, however, because the extensions that we have added contain binaries that are architecture dependent. For example the jupyter-adapter extension uses an architecture-specific build of ZeroMQ, and the positron-r extension contains an architecture-specific build of the ark kernel.

In VS Code 1.80, a change was made (which I have thus far been unable to pinpoint) which causes further divergence of the arm64 and x86 files, such that even the minified Javascript is different between the builds.

Error: Expected all non-binary files to have identical SHAs when creating a universal build but "Contents/Resources/app/extensions/emmet/dist/node/emmetNodeMain.js" did not
    at exports.makeUniversalApp (/Users/user229818/actions-runner/_work/positron/positron/build/node_modules/vscode-universal-bundler/dist/cjs/index.js:73:23)
    at runMicrotasks (<anonymous>)

In order to update to 1.80 and beyond, we need to sort out our build system so that it is more like VS Code's, while still accommodating our use case. Here are some ideas:

juliasilge commented 7 months ago

I believe we have addressed this, especially given that we are currently on 1.85 for VS Code:

https://github.com/posit-dev/positron/blob/5fac682160d7dee912fecdc9a2f826e89846baef/package.json#L3

Is there any remaining work to do?

jmcphers commented 7 months ago

We never did this work -- VS Code reversed the change upstream so now it doesn't appear to be necessary any more.

It would still be a beneficial change:

So I think we should leave this open as a "nice to have"/reference if it comes up again.

Which it could.

jmcphers commented 7 months ago

Updated title to better reflect this.