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

Scripts: Add guestagent and trivy into wsl-distro tarball instead of installing at runtime #7145

Merged
merged 7 commits into from
Jul 11, 2024

Conversation

mook-as
Copy link
Contributor

@mook-as mook-as commented Jul 4, 2024

Fixes #7137.

  • Convert the go utility building to Dependencys, and implement crude dependency tracking so that they can depend on each other.
    • Note that GoDependency can take an array as the first argument, so new GoDependency(['networking', 'cmd', 'network']) is valid.
  • Embed the guest agent and trivy into the WSL distro. Note that we do not change trivy for Lima-based platforms for now.
  • Mark some files as not needing to be installed (this was from when I was ignoring specific files; I've changed to put the files in a new staging directory instead so not packaging them is easier).

This allows dependencies to depend on other dependencies, including across
platforms; we intend to allow WSLDistro to depend on Linux bits.

Signed-off-by: Mark Yen <[email protected]>
This makes the go utilities act like dependencies, so we can use them as
dependencies of other dependencies.

Signed-off-by: Mark Yen <[email protected]>
This embeds rancher-desktop-guestagent as part of the WSL distro on post-
install, so that we do not need to copy it into the VM at run time.

Signed-off-by: Mark Yen <[email protected]>
- We don't actually support localization
- There is no point in including source maps, we don't ship the sources
- Support merging file lists

Signed-off-by: Mark Yen <[email protected]>
@gunamata gunamata requested a review from Nino-K July 5, 2024 16:51
const outFile = this.outFile(context);

console.log(`Building go utility \x1B[1;33;40m${ this.name }\x1B[0m from ${ sourceDir } to ${ outFile }...`);
await simpleSpawn('go', ['build', '-ldflags', '-s -w', '-o', outFile, '.'], {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to handle any potential errors here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any errors that return a non-zero exit code would throw an exception here, so it should be fine.


async download(context: DownloadContext): Promise<void> {
// Build the extension proxy image.
const workDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'rd-build-rdx-pf-'));
Copy link
Member

Choose a reason for hiding this comment

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

Does the fs.promise methods also need error handling, I am just confirming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because they throw exceptions (which will fall over correctly).

const { size } = await fs.promises.stat(fromPath);
const inputFile = fs.createReadStream(fromPath);

console.log(`WSL Distro: Adding ${ fromPath } to ${ name }...`);
Copy link
Member

Choose a reason for hiding this comment

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

nit; I noticed on line 172 it is spelled WSLDistro and here is WSL Distro, we should probably just use one of them to unify the logs.


override environment(context: DownloadContext) {
return {
...super.environment(context),
Copy link
Member

Choose a reason for hiding this comment

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

Curious if we need GOOS here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

super.environment() (line 33) sets GOOS.

@@ -127,6 +131,23 @@ class Builder {
break;
}

// If we have files (resp. extraFiles / extraResources) both at the top
Copy link
Member

Choose a reason for hiding this comment

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

Would it be clearer to change this to

// When there are files (e.g., extraFiles or extraResources) specified at both 
// the top-level and platform-specific levels, we need to combine them 
// and place the combined list at the top level. This approach enables us to have 
// platform-specific exclusions, since the two lists are initially processed 
// separately and then merged together afterward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll just grab your text wholesale.

This rewords the comment describing how we need to merge the files lists
for electron-builder because they way they do merging is a union of the two
resultant sets instead of calculating a set based on a union of the rules.

Signed-off-by: Mark Yen <[email protected]>
This should make it easier to handle custom paths / subcommands.

For example, we may want:

new goUtils.GoDependency('networking/cmd/vm', { outputPath: 'staging/vm-switch'})

Signed-off-by: Mark Yen <[email protected]>
Copy link
Contributor Author

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

I updated GoDependency to better support multiple commands in one package; see the last commit message for details.

const outFile = this.outFile(context);

console.log(`Building go utility \x1B[1;33;40m${ this.name }\x1B[0m from ${ sourceDir } to ${ outFile }...`);
await simpleSpawn('go', ['build', '-ldflags', '-s -w', '-o', outFile, '.'], {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any errors that return a non-zero exit code would throw an exception here, so it should be fine.


override environment(context: DownloadContext) {
return {
...super.environment(context),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

super.environment() (line 33) sets GOOS.


async download(context: DownloadContext): Promise<void> {
// Build the extension proxy image.
const workDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'rd-build-rdx-pf-'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because they throw exceptions (which will fall over correctly).

@@ -127,6 +131,23 @@ class Builder {
break;
}

// If we have files (resp. extraFiles / extraResources) both at the top
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll just grab your text wholesale.

@mook-as mook-as requested a review from Nino-K July 8, 2024 21:14
@Nino-K Nino-K merged commit e07b22e into rancher-sandbox:main Jul 11, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scripts: Add guestagent and trivy into wsl-distro tarball instead of installing at runtime
2 participants