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

build: add watch/compile tasks for CLI #182344

Merged
merged 4 commits into from
Jun 20, 2023
Merged

Conversation

connor4312
Copy link
Member

I spent time this morning working on the 'developer experience' of the
CLI in vscode, mainly getting the CLI to cross-compile chasing our
initial idea of having it auto-build in a devcontainer.

After some effort and disabling tunnels connections (to avoid having to
pull in OpenSSL which is a huge pain to cross compile), I was able to
get it to cross-compile from Linux to Windows, using the mingw linker.
I could probably figure out how to get macOS working as well with more
effort. However, I'm not a big fan of this, effectively it's one more
'platform' of build we need to support and test.

I think a better approach is downloading the latest compiled CLI from
the update server instead, as needed. That's what this PR does. It just
places the CLI where it would normally get compiled to by cargo; so
far we don't need to do anything special outside of that.
A notice is shown to users if this fallback happens.

I spent time this morning working on the 'developer experience' of the
CLI in vscode, mainly getting the CLI to cross-compile chasing our
initial idea of having it auto-build in a devcontainer.

After some effort and disabling tunnels connections (to avoid having to
pull in OpenSSL which is a huge pain to cross compile), I was able to
get it to cross-compile from Linux to Windows, using the mingw linker.
I could probably figure out how to get macOS working as well with more
effort. However, I'm not a big fan of this, effectively it's one more
'platform' of build we need to support and test.

I think a better approach is downloading the latest compiled CLI from
the update server instead, as needed. That's what this PR does. It just
places the CLI where it _would_ normally get compiled to by cargo; so
far we don't need to do anything special outside of that.
A notice is shown to users if this fallback happens.
@connor4312 connor4312 self-assigned this May 12, 2023
@connor4312 connor4312 requested a review from aeschli May 12, 2023 21:22
@vscodenpa vscodenpa added this to the May 2023 milestone May 13, 2023
@aeschli
Copy link
Contributor

aeschli commented May 15, 2023

The downloaded cli is from the latest insiders build and won't match the checked out sources. So while convenient in some cases, this is not clean and will confuse.

  • When you check out some branch, let's say release/1.60 you want to build code oss for exactly these bits.
  • We don't want to serve code insider bits from code oss. Think of the products that are forks of code oss.

I find that requiring developers to install cargo is fine. Building is quite smooth. It's just openssl on Windows that is a pain.

Is there any chance we can consume the part that needs openSSL as a library that we can build in a different repo? Ideally we could extract the whole basis-tunnel part into a library.

Or can we compile the tunnel part only if the developer has the keys tunnelApplicationName and tunnelApplicationConfig in product.json? Then most developers would not need to compile openssl.

FYI @joaomoreno

@joaomoreno
Copy link
Member

I agree with @aeschli. This doesn't resonate at all with me:

a better approach is downloading the latest compiled CLI from
the update server instead, as needed.

In terms of:

It's just openssl on Windows that is a pain.

How could we make it less of a pain?

@connor4312
Copy link
Member Author

If we're okay requiring developers to install Cargo, it gets a good bit easier. I can have a build flag that uses the "vendored" version of cargo and removes the need to linking against an external openssl version, which is what we do in builds by default. Alternatively, we could publish our openssl libraries to npm. Nothing secret about them really. Then we can automatically pull them in in build.rs.

A goal I had was to enable developers to build without having cargo at all if they never touch rust code, in anticipation of a world where the CLI becomes more than just the tool for tunnels. That was the motivating factor behind initially looking at Docker, and then trying this approach instead.

@connor4312
Copy link
Member Author

connor4312 commented May 23, 2023

In this latest build, I take the approach of getting OpenSSL on the fly from the @vscode/openssl-prebuilt package on npm if an openssl-looking error is encountered during compilation. For me this is sufficient to be able to build the CLI with a simple install of Rust without additional setup. I removed the automatic download code.

@connor4312 connor4312 force-pushed the connor4312/local-dev-experience branch from 412028d to ff0c299 Compare May 23, 2023 19:22
@connor4312 connor4312 enabled auto-merge (squash) May 24, 2023 18:20
@connor4312 connor4312 modified the milestones: May 2023, June 2023 Jun 1, 2023
@connor4312 connor4312 merged commit 2138622 into main Jun 20, 2023
@connor4312 connor4312 deleted the connor4312/local-dev-experience branch June 20, 2023 21:34
@github-actions github-actions bot locked and limited conversation to collaborators Aug 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants