Update build process to include integrated CLI #161640

connor4312 · 2022-09-23T20:36:55Z

Adds an off-by-default option that controls whether to build the Rust CLI
The "standalone cli" is built first in parallel, and then mixed into the standard builds. The binary name is suffixed with the quality, e.g. code-tunnel-exploration
The build process is largely based on the vscode-cli repo, but with a new prepare.ts script that sets variables for product.json and version information
Adds a compile and unit test run to PR builds
Updates to use hard tabs instead of spaces as Joao mentioned in the initial PR.

Example pipeline run: https://dev.azure.com/monacotools/Monaco/_build/results?buildId=186450&view=results

joaomoreno

Not yet a full review, but just an observation from a chat discussion: we should zip these up before they end up in our storage and further eat at our CDN costs.

zipping cli-alpine-x64 makes the asset go from 10MB to 4MB

joaomoreno · 2022-09-26T15:50:49Z

Marking as draft, pending a full review early October.

joaomoreno

General comments:

Injecting targets/artifact names from product-build.yml makes the pipelines harder to reason about. Let's be less smart and just describe each platform/arch in the specific *.yml files.
Instead of build/cli directory, it's best to have cli in the names of the pipelines, as in build/darwin/cli-compile-darwin, build/cli-prepare.yml, build/cli-test.yml, etc.
Don't use macos, but darwin.
The whole vspkg usage looks very strange to me. Why is this necessary? Can we abstract this whole thing away in Cargo? If this is only about fetching openssl, can't we manually do that instead? How is this openssl usage tracked in terms of OSS licensing, component governance, Third-Party-Notices, etc?

General notes:

It would have been helpful to have reviewed the whitespace changes in a separate PR.

build/lib/util.ts

build/azure-pipelines/product-build.yml

joaomoreno · 2022-10-05T11:50:08Z

build/azure-pipelines/product-build.yml

+                    - ${{ if eq(parameters.VSCODE_BUILD_WIN32, true) }}:
+                      - x64-windows-static-md
+                    - ${{ if eq(parameters.VSCODE_BUILD_WIN32_ARM64, true) }}:
+                      - arm64-windows-static-md
+                  vcpkgDir: $(Build.SourcesDirectory)/build/azure-pipelines/cli/vcpkg


What about 32-bit Windows? And what if 32-bit Windows is the only Windows platform being built? This targets array will be empty. Should these steps run at all?

I just have not yet tested win32, though it should(?) work. That is on my plate for this month, and I can try that in this PR if you want; my goal was to get initial build bits migrated and in without changes to the CLI itself.

build/azure-pipelines/product-build.yml

build/azure-pipelines/cli/prepare.ts

build/azure-pipelines/cli/install-rust.yml

build/azure-pipelines/cli/vcpkg-deps.yml

build/azure-pipelines/common/createAsset.ts

connor4312 · 2022-10-05T15:46:52Z

The whole vspkg usage looks very strange to me. Why is this necessary? Can we abstract this whole thing away in Cargo? If this is only about fetching openssl, can't we manually do that instead? How is this openssl usage tracked in terms of OSS licensing, component governance, Third-Party-Notices, etc?

We use this to grab a Windows build of OpenSSL. This is currently required by our SSH crate, russh, for its cryptography. However I soon plan to move this to either use native Windows wincrypt.h, or pure Rust code, depending on outcome from discussion with the crypto board. CG is aware of vcpkg (which is a Microsoft tool). I think we have to update the OSS tool's ThirdPartyNotices.txt generator to parse the Cargo.lock anyway, I will add that to my todo.

build/azure-pipelines/product-build.yml

build/azure-pipelines/cli/prepare.ts

joaomoreno

Thanks for simplifying, we're getting there!

connor4312 · 2022-10-10T16:46:42Z

What happened to this code?

Apparently I can't comment on it, but: my plan for that is to make the necessary Cargo.toml changes in vscode-distro and have them merge in. But I will do that in a followup PR (in distro) once this is in to avoid conflicts.

joaomoreno

Last round. Happy to approve and merge this once rest of comments are addressed. ✅

build/azure-pipelines/cli/install-rust-posix.yml

build/azure-pipelines/cli/install-rust-win32.yml

build/azure-pipelines/cli/cli-darwin-sign.yml

build/azure-pipelines/cli/cli-compile-and-publish.yml

build/azure-pipelines/linux/cli-build-linux.yml

build/azure-pipelines/product-build-pr.yml

build/azure-pipelines/win32/product-build-win32.yml

connor4312 added 4 commits September 19, 2022 23:16

build: include the integrated CLI in product builds

e028959

Merge branch 'connor4312/import-cli' into build-integrated-cli

5e2ce13

cli: get builds in order

9bcc88e

Merge remote-tracking branch 'origin/main' into build-integrated-cli

665eb03

connor4312 requested a review from joaomoreno September 23, 2022 20:37

connor4312 self-assigned this Sep 23, 2022

connor4312 requested a review from aeschli September 23, 2022 20:37

vscodenpa added this to the September 2022 milestone Sep 23, 2022

connor4312 added 2 commits September 23, 2022 14:17

cli: use hard tabs to align with vscode style

b784bcd

cli: run ci on pr

a9bcb15

aeschli requested a review from lszomoru September 26, 2022 13:18

joaomoreno reviewed Sep 26, 2022

View reviewed changes

joaomoreno removed request for lszomoru and aeschli September 26, 2022 15:42

joaomoreno modified the milestones: September 2022, October 2022 Sep 26, 2022

joaomoreno marked this pull request as draft September 26, 2022 15:50

Merge branch 'main' into build-integrated-cli

081133f

joaomoreno self-requested a review October 5, 2022 11:25

joaomoreno requested changes Oct 5, 2022

View reviewed changes

connor4312 added 3 commits October 5, 2022 10:00

Merge remote-tracking branch 'origin/main' into build-integrated-cli

cbc174d

address pr comments

b401701

address pr comments

18f743d

joaomoreno reviewed Oct 10, 2022

View reviewed changes

build/azure-pipelines/product-build.yml Outdated Show resolved Hide resolved

build/azure-pipelines/product-build.yml Outdated Show resolved Hide resolved

build/azure-pipelines/cli/prepare.ts Outdated Show resolved Hide resolved

joaomoreno reviewed Oct 10, 2022

View reviewed changes

connor4312 added 2 commits October 10, 2022 10:39

address pr comments

1b11bc6

address pr comments

191dc77

fix build

dc69e22

connor4312 force-pushed the build-integrated-cli branch from 46a9ad0 to fba577c Compare October 11, 2022 01:46

Merge branch 'build-integrated-cli-dev' into build-integrated-cli

ebd5936

connor4312 force-pushed the build-integrated-cli branch from fba577c to ebd5936 Compare October 11, 2022 02:13

joaomoreno requested changes Oct 11, 2022

View reviewed changes

address pr comments

b42f25b

connor4312 marked this pull request as ready for review October 11, 2022 15:42

joaomoreno approved these changes Oct 12, 2022

View reviewed changes

connor4312 merged commit 6353f80 into main Oct 12, 2022

connor4312 deleted the build-integrated-cli branch October 12, 2022 15:59

github-actions bot locked and limited conversation to collaborators Nov 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update build process to include integrated CLI #161640

Update build process to include integrated CLI #161640

connor4312 commented Sep 23, 2022 •

edited

Loading

joaomoreno left a comment

joaomoreno commented Sep 26, 2022

joaomoreno left a comment •

edited

Loading

joaomoreno Oct 5, 2022

connor4312 Oct 5, 2022

connor4312 commented Oct 5, 2022 •

edited

Loading

joaomoreno left a comment

connor4312 commented Oct 10, 2022 •

edited

Loading

joaomoreno left a comment

Update build process to include integrated CLI #161640

Update build process to include integrated CLI #161640

Conversation

connor4312 commented Sep 23, 2022 • edited Loading

joaomoreno left a comment

Choose a reason for hiding this comment

joaomoreno commented Sep 26, 2022

joaomoreno left a comment • edited Loading

Choose a reason for hiding this comment

joaomoreno Oct 5, 2022

Choose a reason for hiding this comment

connor4312 Oct 5, 2022

Choose a reason for hiding this comment

connor4312 commented Oct 5, 2022 • edited Loading

joaomoreno left a comment

Choose a reason for hiding this comment

connor4312 commented Oct 10, 2022 • edited Loading

joaomoreno left a comment

Choose a reason for hiding this comment

connor4312 commented Sep 23, 2022 •

edited

Loading

joaomoreno left a comment •

edited

Loading

connor4312 commented Oct 5, 2022 •

edited

Loading

connor4312 commented Oct 10, 2022 •

edited

Loading