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(cli): warn about bundling updater target without appropriate targets, closes #7181 #7189

Merged
merged 3 commits into from
Jun 13, 2023

Conversation

amrbashir
Copy link
Member

@amrbashir amrbashir commented Jun 13, 2023

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@amrbashir amrbashir requested a review from a team as a code owner June 13, 2023 15:33
@lucasfernog
Copy link
Member

On Linux and macOS we're actually triggering the AppImage/app bundling. We just need to do the same on Windows.

None => appimage::bundle_project(settings)?,

None => app::bundle_project(settings)?,

@amrbashir
Copy link
Member Author

I knew I saw this code somewhere lol.

Anyways, while this works fine for Linux and macOS, it is not the same case for Windows since there is two updater targets and not just one, so should we build both targets? doesn't sound intuitive to me, instead we should be disabling building updater target without another updater-enabled target, what do you think?

@lucasfernog
Copy link
Member

What if we assume NSIS in this case? I think that should be our focus going forward, and maybe we can even drop WiX on v2 (probably move it out of the monorepo so custom users have time to migrate).

@amrbashir
Copy link
Member Author

We can assume NSIS as default for now but it is not future-proof, if we decided to add another bundler target like Squirrel or maybe MSIX

@lucasfernog
Copy link
Member

Hmm ok. Safer to go with this idea then.

@amrbashir amrbashir merged commit d75c1b8 into dev Jun 13, 2023
38 checks passed
@amrbashir amrbashir deleted the fix/cli/lonely-updater branch June 13, 2023 23:14
@chippers chippers added the security: reviewed This issue/PR has been review by wg-security label Jun 16, 2023
Comment on lines +100 to +113
PackageType::Updater => {
if !package_types.iter().any(|p| {
matches!(
p,
PackageType::AppImage
| PackageType::MacOsBundle
| PackageType::Nsis
| PackageType::WindowsMsi
)
}) {
warn!("The updater bundle target exists but couldn't find any updater-enabled target, so the updater artifacts won't be generated. Please add one of these targets as well: app, appimage, msi, nsis");
continue;
}
updater_bundle::bundle_project(&settings, &bundles)?
Copy link
Contributor

Choose a reason for hiding this comment

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

I have just found out that package_types is never updated when a macOS DMG bundle is generated (see lines 77-85). Therefore, the updater bundle will not be generated if the macOS .app bundle is not explicitly set in package_types, even though the DMG bundle implies the generation of an app bundle and thus package_types does not need to explicitly contain a macOS .app bundle.

I'd suggest adding a | PackageType::Dmg branch to the matches! macro here to better handle this edge case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security: reviewed This issue/PR has been review by wg-security
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants