forge icon indicating copy to clipboard operation
forge copied to clipboard

fix: Prevent conflict between parallel makers

Open macdja38 opened this issue 1 year ago • 6 comments

At the moment maker-dmg will simultaneously try to write to out/make/AppName.dmg. This commit changes that to out/make-darwin-arm64/AppName.dmg preventing any conflicts

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

Summarize your changes: at the moment maker-dmg fails when run in parallel, for an explanation on why / how / the details see here: https://github.com/electron/forge/issues/3517

This solution: We pass a different path into the maker from core for each platform & arch combination, which prevents it from trying to use the same file in an intermediary step.

Downsides: People might rely on the location of the dmg & zip to be in out/make directly, this would obviously ruin that.

Alternatives considered: Potentially it would be possible to get maker-dmg to generate a DMG with the required name from the start instead of always generating it with name.dmg and then renaming to name-version-arch.dmg.

code for context maker-dmg/src/MakerDMG.ts

    await this.ensureFile(outPath);
    const dmgConfig = {
      overwrite: true,
      name: appName,
      ...this.config,
      appPath: path.resolve(dir, `${appName}.app`),
      out: path.dirname(outPath),
    };
    const opts = await electronDMG(dmgConfig);
    if (!this.config.name) {
      await this.ensureFile(forgeDefaultOutPath);
      await fs.rename(outPath, forgeDefaultOutPath);
      return [forgeDefaultOutPath];
    }

    return [opts.dmgPath];
    ```

macdja38 avatar Feb 29 '24 20:02 macdja38

Someone asked me about this on Discord last week and I agree with the above. As you stated in the downsides section in the description, I think this too likely to catch people off-guard.

erickzhao avatar Mar 05 '24 22:03 erickzhao

@MarshallOfSound the fallback I mentioned won't work if the user supplies a name via config (though the may have been broken for a long time when building for multiple arches, not sure).

For now I think it's out of scope of this PR to fix that, but I'll leave that up to you. I'm thinking ideally we'd allow passing a function or string for the name instead of just a string.

current:

{
  name: "@electron-forge/maker-dmg",
  config: {
    name: "AppName",
    overwrite: true,
  },
  platforms: ["darwin"],
},

(this would break because we can't write two AppName.dmg to the same out/make folders. I believe this would have broken before this PR and before 7.0 as well though)

Proposed ability to pass function

{
  name: "@electron-forge/maker-dmg",
  config: {
    name: (version: string, platform: string, arch: string) => `AppName-${arch}-${platform}`,
    overwrite: true,
  },
  platforms: ["darwin"],
}

(idk if we'd want version or not, the user can get that themselves)

macdja38 avatar Mar 06 '24 23:03 macdja38

Updated the PR, also removed the two tests that tested renaming as we don't need to rename anymore.

macdja38 avatar Mar 07 '24 01:03 macdja38

@MarshallOfSound is there any chance you could take another look?

macdja38 avatar Mar 21 '24 00:03 macdja38

Updated to resolve merge conflicts

macdja38 avatar May 07 '24 16:05 macdja38

How about MakerSquirrel or MakerAppImage to solve this problem?

❯ Making a squirrel distributable for win32/x64
❯ Making a squirrel distributable for win32/arm64
✖ Making a squirrel distributable for win32/x64 [FAILED: Failed with exit code: 4294967295
Output:
Couldn't open log file, trying new file: System.IO.IOException: The process cannot access the file 'D:\a\Follow\Follow\node_modules\electron-winstaller\vendor\Squirrel-Releasify.log' because it is being used by another process.
❯ Making a appImage distributable for linux/x64
❯ Making a appImage distributable for linux/arm64
  ⨯ symlink usr/share/icons/hicolor/256x256/apps/Follow.png /home/runner/work/Follow/Follow/out/make/__appImage-x64/Follow.png: file exists
github.com/develar/app-builder/pkg/package-format/appimage.copyIcons.func1.1
	/Volumes/data/Documents/app-builder/pkg/package-format/appimage/appLauncher.go:82
github.com/develar/app-builder/pkg/util.MapAsyncConcurrency.func2
	/Volumes/data/Documents/app-builder/pkg/util/async.go:68
runtime.goexit
	/usr/local/Cellar/go/1.17/libexec/src/runtime/asm_amd64.s:1581  
✖ Making a appImage distributable for linux/arm64 [FAILED: /home/runner/work/Follow/Follow/node_modules/app-builder-bin/linux/x64/app-builder process failed ERR_ELECTRON_BUILDER_CANNOT_EXECUTE
Exit code:
1]
✖ Making distributables [FAILED: /home/runner/work/Follow/Follow/node_modules/app-builder-bin/linux/x64/app-builder process failed ERR_ELECTRON_BUILDER_CANNOT_EXECUTE

Innei avatar Aug 13 '24 07:08 Innei