Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

lockiechen
Copy link

@lockiechen lockiechen commented Oct 21, 2024

  • 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:

@lockiechen lockiechen requested a review from a team as a code owner October 21, 2024 03:12
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

I think we should mirror the other makers and do {makeDir}/{makerName}/{platform}/{arch}/{file}

E.g. MakerZIP https://github.com/electron/forge/blob/main/packages/maker/zip/src/MakerZIP.ts#L42

const { createDMG } = require('electron-installer-dmg');

const outPath = path.resolve(makeDir, `${this.config.name || appName}.dmg`);
const dmgName = `${this.config.name || appName}_${targetArch}`;
Copy link
Member

Choose a reason for hiding this comment

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

Will this mean that all DMGs will have the arch suffix going forward?

  1. I'd much prefer my DMGs not to have that extension.
  2. I assume it would be a breaking change for some people. The way my update server is written, it would break if the DMG name suddenly changes.

IMO it should be configurable whether to have the arch in the file name (if it's even an option at all).

Copy link
Author

Choose a reason for hiding this comment

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

ok, i will provide a option to enable or not

@erickzhao erickzhao changed the title fix: Prevent conflict between parallel makers #3519 fix(maker-dmg): Prevent conflict between parallel makers #3519 Dec 14, 2024
@erickzhao
Copy link
Member

I think I like @MarshallOfSound's approach here but I guess that would be a breaking change for users. Down to merge this when we bump our packages up to Node 22 and have a Forge 8 release.

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.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.