-
Notifications
You must be signed in to change notification settings - Fork 245
Add sandboxing and refactored smoke test harness #6621
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
Conversation
| .catch(function (err) { | ||
| console.error(err.stack); | ||
| process.exit(1); | ||
| process.exitCode = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://nodejs.org/api/process.html#processexitcode:
Rather than calling process.exit() directly, the code should set the process.exitCode and allow the process to exit naturally by avoiding scheduling any additional work for the event loop:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is like I know this and keep independently rediscovering it yet still forget.
| 'IS_RHEL', | ||
| ]) | ||
| ); | ||
| writeBuildInfo(infoArgs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of me wants to change hadron-build so we can get this info as a variable without an intermediate file, but I've been a bit weary of changing that code because it is used in so many ways and often only at release time. Although I suppose nowadays we release dev too, so it should be safer.
| filepath, | ||
| destinationPath: context.sandboxPath, | ||
| }); | ||
| cleanups.push(uninstall); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably uninstall before clearing out the sandbox because if it runs an uninstaller that uninstaller will be inside the sandbox.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - deleting the sandbox should be the last cleanup task 👍
lerouxb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
| import { type fetch as undiciFetch } from 'undici-types'; | ||
|
|
||
| // Hacking types here because the DOM types doesn't match the fetch implemented by Node.js | ||
| const fetch = globalThis.fetch as unknown as typeof undiciFetch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be avoided if we moved the code into its own package with types properly locked down.
Description
Merging this PR will:
downloadFileto usefetchand avoid having to implement following redirection manually and adopt to Anna's suggestion of usingstream.promises.pipeline.skipDownload(which had two purposes - testing locally built / packaged compass as well as spending less time downloading) with a download cache based on theETagheader of the server response and a new--forceDownloadflag to clear the cache before download.dist/target.jsonwith "build info" is produced, so to test a locally built and packaged Compass, we don't need to invoke Hadron build again. I've added a--localPackageto skip hadron-build and download entirely and instead use thecompass/distdirectory (and its artifacts).if (kind === 'osx_dmg') { console.log(buildInfo.osx_dmg_filename)(i.e. a conditional on the kind of package narrows buildInfo). I've added the extraction of the filename from the "build info" based on the kind of package as well.Checklist
Motivation and Context
Open Questions
Dependents
Types of changes