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
feat: remove qq and parallelize build/pack/promote #1035
Conversation
@@ -18,7 +17,6 @@ This can be used to create oclif CLIs that use the system node or that come prel | |||
} | |||
|
|||
async run(): Promise<void> { | |||
const prevCwd = qq.cwd() |
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.
@mshanemc why is managing cwd no longer necessary?
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.
all the paths are now either absolute or relative to the project.
Use of process.exec instead of qq sometimes passes in a cwd (still absolute or relative to the project).
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.
nothing ever changes the cwd, which is what was breaking the previous attempt at parallelization
* test: integration tests in parallel * test: restore debian integration tests
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.
.DS_Store
shouldn't be checked in. Please remove and add to .gitignore
.
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.
Did you do any testing on the promote command? I know integration tests don't exist for it currently but might be nice to add some with all the changes going in.
Co-authored-by: Rodrigo Espinosa de los Monteros <1084688+RodEsp@users.noreply.github.com>
just manually. I checked it again with the new changes via. ../oclif-repos/oclif-oclif/bin/dev promote --channel qqqa --version 7.179.0 --sha 4383be1 I'm looking into the integration test options |
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.
Looks good! Just one small comment but doesn't need to be changed to merge.
Comments were addressed. Re-review please.
* feat: remove qq and parallelize build/promote * chore: reformat pjson * refactor: remove redundant objects * refactor: pr feedback * feat: publishes debian packages with qq * test: integration tests in parallel * test: restore debian integration tests * chore: remove ds_store * chore: ignore dsStore * chore: ignore dist * Update src/commands/upload/deb.ts Co-authored-by: Rodrigo Espinosa de los Monteros <1084688+RodEsp@users.noreply.github.com> * fix: cwd on pretarball * refactor: pr feedback * test: integration test for tarball promote * test: revert unnecessary trims * perf: optional xz hashes in parallel * chore: fix pjson format * refactor: remove unused conditional * fix: osslsign command requires "sign" Co-authored-by: Rodrigo Espinosa de los Monteros <1084688+RodEsp@users.noreply.github.com>
// this workaround puts the code in both places that the redirect was doing | ||
// with this, the docs are correct. The copies are all done in parallel so it shouldn't be too costly. | ||
const uploadWorkaround = (file: string) => { | ||
const cloudKey = `${cloudKeyBase}/apt/./${file}` |
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.
Removing the ./
fixes the issue here
we had to drop the --parallel flag on builds because of a bug.
remove
qqjs
and itsqq.cd
that's causing the problem with parallelization.make sure each build process has its own directory isolation
parallelize all the things
@W-12107459@
side effects
fix #347
test: restores integration testing for Debian builds (comment said they weren't runnable on CircleCI. Works on gha)
test: runs integration tests in parallel on different OSes to test both mac and deb packing
perf metrics [mac, 2.4 GHz 8-Core Intel Core i9]
rm -rf dist test/tmp tmp
on gha builds (2 core machine). [mac tests can't run because they only run on macs]