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

cli: automatically use distro qualities if available #166961

Merged
merged 2 commits into from Nov 22, 2022

Conversation

connor4312
Copy link
Member

@connor4312 connor4312 commented Nov 22, 2022

A fully-functioning CLI with tunnel capabilities requires product-specific configuration that is not included in the OSS. This change has the CLI's build script automatically look for a peer vscode-distro folder and, in a debug build, set its build variables based on that, to make development easier.

It also works to set correct variables if vscode-distro is not found, based on the OSS product.json (though this is not sufficient to run a fully fledged tunnel.)

A fully-functioning CLI with tunnel capabilities requires product-specific
configuration that is not included in the OSS. This change has the CLI's build
script automatically look for a peer `vscode-distro` folder and, in a debug
build, set its build variables based on that.

It also works to set correct variables if vscode-distro is not found, based on
the OSS product.json (though this is not sufficient to run a fully fledged
tunnel.)
@connor4312 connor4312 merged commit 94ee5f5 into main Nov 22, 2022
@connor4312 connor4312 deleted the connor4312/cli-improve-dev-build branch November 22, 2022 14:13
@aeschli
Copy link
Contributor

aeschli commented Nov 23, 2022

@connor4312 I'm not a fan of that solution of adding knowledge of vscode-distro to the build scripts and accessing it via ../vscode-ditro.
One problem is that we can't just take whatever is checked out in ../vscode-distro but we need the distro commit as specified in package.json.
Second is that @joaomoreno is working or refactoring the way distro is mixed in. The build scripts in OSS should not depend on that, They just use the product.json available in the repo root.

I think, if we need to add knowledge about other qualities, we should add it to product.json, in tunnelApplicationConfig (for now). @joaomoreno wanted to chat about that (he might be on vacation right now)

@connor4312
Copy link
Member Author

connor4312 commented Nov 23, 2022

One problem is that we can't just take whatever is checked out in ../vscode-distro but we need the distro commit as specified in package.json.

I think this is mostly up to the developer, if they see functionality not working then they should check distro is synced correctly. But the developer might also want to test or verify changes in distro on a commit different than what's in the package.json. I kind of viewed this as a 'special' config similar to the optional install of the vsda dependency.

I think, if we need to add knowledge about other qualities, we should add it to product.json, in tunnelApplicationConfig

We depend on a sizable amount of information about other qualities since any given install of the CLI must interact with them. The six win32*AppId properties, nameLong, applicationName, serverApplicationName, and downloadUrl from each quality are currently mixed into the CLI. I don't think it is a good idea to duplicate this information inside every tunnelApplicationConfig; that could get out of sync and broken very easily.

Also, as mentioned in DM, when developing the CLI I do not want to have to make local product.json changes to produce a usable build, since it is very easily to accidentally commit those changes. I would be very happy to hear other alternative development solutions 🙂

lemanschik pushed a commit to code-oss-dev/code that referenced this pull request Nov 25, 2022
A fully-functioning CLI with tunnel capabilities requires product-specific
configuration that is not included in the OSS. This change has the CLI's build
script automatically look for a peer `vscode-distro` folder and, in a debug
build, set its build variables based on that.

It also works to set correct variables if vscode-distro is not found, based on
the OSS product.json (though this is not sufficient to run a fully fledged
tunnel.)
@github-actions github-actions bot locked and limited conversation to collaborators Jan 6, 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.

None yet

4 participants