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

fix(packager): "packaging application" log never stops when building for multiple architectures #3006

Merged
merged 4 commits into from
Oct 31, 2022

Conversation

macdja38
Copy link
Contributor

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

Issue / minimal test case available here

There's some kinda complex race condition ish behaviour in the current way the packagerSpinner is handled.

At the moment there's one variable for all architectures, that gets created in one of the later afterCopyHooks, and dismissed both in the first hook, and at the end of all the hooks. This would work if architectures where built for sequentially, but, at least on my system, these builds happen in parallel. Meaning one of the architectures packageSpinners overwrites the other, before the first is dismissed. At the end of the build the second packageSpinner has success fired on it, while the first continues to spin, preventing the process from exiting.

The solution in this commit is far less than ideal.

Constraints:

  • Without access to the list of architectures (the function to generate that list is not exported from electron-packer) we don't even know how many architectures we're building for.
  • This one is a little self-imposed, and maybe not a constraint for the project, but the code aims not to change any public facing apis. So we need to have one spinner we can pass to the postPackage hook. https://www.electronforge.io/configuration#postpackage

Given these constraints the solution in this commit has one initial prepare / package spinner. AS well as prepare / package spinners per architecture.

One downside is if the afterCopyHooks are executed one architecture at a time, instead of parallelized by architecture, the overarching prepareSpinner could be ended early, (though architecture specific prepare spinners would still be accurate).

…iple architectures

There's some kinda complex race condition ish behaviour in the current way the packagerSpinner is handled.

At the moment there's one variable for all architectures, that gets created in one of the later afterCopyHooks, and dismissed both in the first hook, and at the end of all the hooks. This would work if architectures where built for sequentially, but, at least on my system, these builds happen in parallel. Meaning one of the architectures packageSpinners overwrites the other, before the first is dismissed. At the end of the build the second packageSpinner has success fired on it, while the first continues to spin, preventing the process from exiting.

The solution in this commit is far less than ideal.

Constraints:
 - Without access to the list of architectures (the function to generate that list is not exported from electron-packer) we don't even know how many architectures we're building for.
 - This one is a little self-imposed, and maybe not a constraint for the project, but the code aims not to change any public facing apis. So we need to have one spinner we can pass to the postPackage hook. https://www.electronforge.io/configuration#postpackage

 Given these constraints the solution in this commit has one initial prepare / package spinner. AS well as prepare / package spinners per architecture.

One downside is if the afterCopyHooks are executed one architecture at a time, instead of parallelized by architecture, the overarching prepareSpinner could be ended early, (though architecture specific prepare spinners would still be accurate).
@macdja38
Copy link
Contributor Author

Note: I'm not super happy with the way this came out. I'd really prefer to keep state around what's going on and assemble clear strings / limit us to one spinner at any time. Without the validateListFromOptions function from electron-packager/src/targets.js I'm not sure if there's a better way.

Tests are failing locally for me (I haven't looked into if that's because of my setup or the code changes), but if we want to move forward with this / a variant of this I'll make sure those work.

@MarshallOfSound
Copy link
Member

MarshallOfSound commented Oct 27, 2022

ora does not work with multiple spinners at the same time, so I like your state management here but I don't think the solution is "also keep track of multiple spinners".

I think the better solution looks something like this (psuedocode)

  1. package(targets)
  2. Spinner Packaging for {archs.join(', ')}
  3. As each arch finishes
  • Complete the current spinner with the text Packaging for {archThatFinished} complete
  • Start a new spinner with Packaging for {remainingArchs.join(', ')}
  1. When all arches are finished complete the final spinner with the text Packaging complete

We pass the "final spinner" into postPackage.

As for the issue of knowing what arches we're building for, I think it makes sense for packager to help forge out a little here. I think a new hook in packager postFinalizeTargets or something (idk, naming is not my strong suit) that can tell the caller the matrix of targets that we will be packaging for is the right way forward.

Thanks for tackling this one head on 😅

@MarshallOfSound
Copy link
Member

As for the issue of knowing what arches we're building for

Though to be fair actually thinking about using --arch=all with forge might have other issues anyway 🤔 Never tried it tbh

@macdja38
Copy link
Contributor Author

I used to be able to build with --arch=all, but at some point, maybe after upgrading an electron version, maybe after upgrading from beta 63 to beta 68 of electron forge, that broke and I had to switch to --arch=arm64,x64.

@macdja38
Copy link
Contributor Author

As for the issue of knowing what arches we're building for, I think it makes sense for packager to help forge out a little here. I think a new hook in packager postFinalizeTargets or something (idk, naming is not my strong suit) that can tell the caller the matrix of targets that we will be packaging for is the right way forward.

Is this something that I / We / You can get added?

@MarshallOfSound
Copy link
Member

@macdja38 I think this should let us do what we need to do:

electron/packager#1437

@macdja38
Copy link
Contributor Author

@MarshallOfSound yup that looks perfect! I'll update this PR to use that tonight! (EST)

@macdja38
Copy link
Contributor Author

Updated to mostly match feedback.

Preparing to package has been mostly removed.

New logs once completed are:

✔ Preparing to Package Application
✔ Preparing native dependencies
✔ Preparing native dependencies
✔ Packaging for arm64 complete
✔ Packaging for x64 complete
✔ Packaging complete

In progress has platforms in it for now.

⠏ Packaging Application for darwin:x64, darwin:arm64

We can definitely remove the platform, not sure if it's helpful or not. (or we can add it to the completed messages)

I included Packaging complete as a spinner, it gives us something nice to pass to the hook, and means that it doesn't look like one specific arch is taking longer if the postPackage step takes a while. Maybe the text should be changed to Post packaging

@MarshallOfSound
Copy link
Member

@macdja38 I took the liberty of rebasing your PR and updating it with the latest released electron-packager 😄

@MarshallOfSound MarshallOfSound merged commit 247f52a into electron:main Oct 31, 2022
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.

2 participants