-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
chore(refactor): explore parallelization of builds #8962
base: master
Are you sure you want to change the base?
Conversation
…`concurrency` config prop
🦋 Changeset detectedLatest commit: 6bcde34 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
looks great! will make efforts to implement this for us! |
@@ -401,7 +410,8 @@ export class NsisTarget extends Target { | |||
|
|||
// https://github.com/electron-userland/electron-builder/issues/2103 | |||
// it is more safe and reliable to write uninstaller to our out dir | |||
const uninstallerPath = path.join(this.outDir, `__uninstaller-${this.name}-${this.packager.appInfo.sanitizedName}.exe`) | |||
// to support parallel builds, the uninstaller path must be unique to each target and arch combination | |||
const uninstallerPath = path.join(this.outDir, `__uninstaller-${Math.floor(Math.random() * Date.now())}-${this.name}-${this.packager.appInfo.sanitizedName}.exe`) |
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.
Will there be any issues during user upgrades or overlay installations? Since the paths change each time, will uninstalling previous versions fail due to missing paths?
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.
I suggest using fixed paths, such as including the type (nsis/squirrel) in the path, rather than generating random paths. Some uninstallation processes require these fixed paths for proper removal.
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 used to be this develar@8738e18
const uninstallerPath = path.join(this.outDir, `.__uninstaller-${this.name}-${process.pid.toString(16)}-${Date.now().toString(16)}.exe`)
I can't seem to track down the commit/PR that changed it to its current form.
The prefix __uninstaller
doesn't exist anywhere else in the codebase either https://github.com/search?q=repo%3Aelectron-userland%2Felectron-builder%20__uninstaller&type=code
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.
The uninstallerPath is passed to install.nsi through UNINSTALLER_OUT_FILE.
defines.UNINSTALLER_OUT_FILE = isWin ? uninstallerPath : path.win32.join("Z:", uninstallerPath) |
!ifdef BUILD_UNINSTALLER | |
WriteUninstaller "${UNINSTALLER_OUT_FILE}" | |
!insertmacro quitSuccess |
Apps registered through registry or in Settings/Apps/Uninstall Apps may have issues, we need to test and verify.
WriteRegStr HKLM "Software\Microsoft\Windows\CurrentVersion\Uninstall\${APP_NAME}" "UninstallString" '"${UNINSTALLER_OUT_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.
If we use a fixed path, then we completely cannot have parallel builds of nsis target.
We could theoretically switch it back to a tmp-dir with a fixed name, but then that goes explicitly against the comment above it: // it is more safe and reliable to write uninstaller to our out dir
The problem is that I can't identify the why based off the issue linked in the 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.
@beyondkmp @xyloflake what if we just had a unique directory name within the out
directory? Then we keep the uninstaller name as a constant? That way we still respect the comment of "reliability outputting to out
dir" while maintaining the static name.
Previous implementations were these, so I think we'd still be satisfying the original intent while not introducing regressions:
packager.getTempFile("uninstaller.exe") // completely temp dir/file elsewhere on the filesystem
```
```
path.join(this.outDir, `.__uninstaller-${this.name}-${process.pid.toString(16)}-${Date.now().toString(16)}.exe`) // within outDir but non-static file name
```
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.
@beyondkmp @xyloflake what if we just had a unique directory name within the
out
directory? Then we keep the uninstaller name as a constant? That way we still respect the comment of "reliability outputting toout
dir" while maintaining the static name.Previous implementations were these, so I think we'd still be satisfying the original intent while not introducing regressions:
packager.getTempFile("uninstaller.exe") // completely temp dir/file elsewhere on the filesystem
path.join(this.outDir, `.__uninstaller-${this.name}-${process.pid.toString(16)}-${Date.now().toString(16)}.exe`) // within outDir but non-static file name
I personally think that is a GREAT idea. I think it should be something like: if concurrency is enabled, then it should use the unique directory in the out folder and if it doesn't have concurrency enabled then it should fallback to this old method. But I think even if there is no fallback, this is a great idea and I think it's pretty necessary for parallelization!
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.
@beyondkmp, thinking on this further, I think we can just use the installerPath
since it should always be unique to each installer being built, regardless of archs since that's calculated previously into the artifact name (installer path)
const uninstallerPath = path.join(this.outDir, `${path.basename(installerPath, "exe")}__uninstaller.exe`)
if (poolCount < 1) { | ||
log.warn({ concurrency: poolCount }, "concurrency is invalid, overriding with job count: 1") | ||
poolCount = 1 | ||
} else if (poolCount > MAX_FILE_REQUESTS) { |
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.
Can delete else
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.
Wait, why?
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.
Wait, why?
Same question, I believe that is a necessary conditional
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.
Apologized for the misunderstanding.
I mean
if (poolCount < 1) {
log.warn({ concurrency: poolCount }, "concurrency is invalid, overriding with job count: 1")
poolCount = 1
}
if (poolCount > MAX_FILE_REQUESTS) {}
However, this is just a coding convention, it's optional to change it.
…atic name convention
LGTM |
Allow targets to be executed in parallel for those that do not include locking of resources (macos dmg: hdiutil, win: squirrel/nsis/msi signing, etc)
Adds a massive ConcurrentBuildsTest suite for making sure all assets are produced correctly and to measure speed improvements against
Implements #8888
CC @xyloflake if you're willing to be an early adopter of the experimental implementation