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: S3 urls on mac #1386

Closed
wants to merge 1 commit into from
Closed

fix: S3 urls on mac #1386

wants to merge 1 commit into from

Conversation

markusdosch
Copy link
Contributor

@markusdosch markusdosch commented Mar 20, 2017

(follow-up to #1373, only an issue for S3 bucket names with dots)

Releasing for macOS results in the following error:

Publishing to S3 (bucket: <bucket-name-with-dots>)
[====================] 100% 0.0s | App-0.2.3.dmg to S3
Error: Bucket name "<bucket-name-with-dots>" includes a dot, but S3 region is missing
    at s3Url (/<...>/desktop-app-prototype/node_modules/electron-builder-http/src/publishOptions.ts:129:13)
    at computeDownloadUrl (/<...>/desktop-app-prototype/node_modules/electron-builder/src/publish/PublishManager.ts:344:15)
    at /<...>/desktop-app-prototype/node_modules/electron-builder/src/publish/PublishManager.ts:241:14
From previous event:
    at writeUpdateInfo (/<...>/desktop-app-prototype/node_modules/electron-builder/out/publish/PublishManager.js:86:22)
    at /<...>/desktop-app-prototype/node_modules/electron-builder/src/publish/PublishManager.ts:137:22
From previous event:
    at PublishManager.artifactCreated (/<...>/desktop-app-prototype/node_modules/electron-builder/out/publish/PublishManager.js:388:11)

=> So, for macOS releases the field options.region is not set automatically!

At the same time, the windows release works (incl. app-update.yml being automatically populated with the bucket's region).

A workaround for the mentioned error is to manually add publish.region to the package.json.

This pull request tries to introduce a fix - but I couldn't test it locally (see bottom of this post for error).

After implementing my path approach, I discovered the line:

if (!event.targets.some(it => it.name === "zip")) {

I imagine that the if condition becomes true in my macOS case which leads to the options.region not being generated and the error takes place. If I'm correct, reformulating this if condition would be a better and simpler bug fix.

Appreciate some feedback.


"Appendix": Error when trying to test locally

Whenever I added my local electron-builder to my project (after running npm run compile on the local electron-builder) and tried to build my local project for macOS, I ran into the following error:

Signing app (identity: Developer ID Application: <...>)
Building macOS zip
Building DMG
Error: Exit code: 255. Command failed: /usr/bin/perl /var/folders/59/j5b7r9d931gb5cxwbqxzl1hc0000gn/T/electron-builder-dSaFRF/0-2-dmgProperties.pl
Error: Error querying file
Cannot query image dimensions at /var/folders/59/j5b7r9d931gb5cxwbqxzl1hc0000gn/T/electron-builder-dSaFRF/0-2-dmgProperties.pl line 22.

Error: Error querying file
Cannot query image dimensions at /var/folders/59/j5b7r9d931gb5cxwbqxzl1hc0000gn/T/electron-builder-dSaFRF/0-2-dmgProperties.pl line 22.

    at /<...>/desktop-app-prototype/node_modules/electron-builder-util/src/util.ts:75:16
    at ChildProcess.exithandler (child_process.js:218:5)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:192:7)
    at maybeClose (internal/child_process.js:890:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:226:5)
From previous event:
    at exec (/<...>/desktop-app-prototype/node_modules/electron-builder-util/src/util.ts:53:3)
    at /<...>/desktop-app-prototype/node_modules/electron-builder/src/targets/dmg.ts:153:7

brew remove perl like suggested in #815 did not fix this error for me.

@mention-bot
Copy link

@Sukram21, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Anmo and @piotrdubiel to be potential reviewers.

@develar
Copy link
Member

develar commented Mar 20, 2017

My fault during last review.

We must call modifyPublishConfig not during addAfterPackHandler but in the getResolvedPublishConfig

And expandPublishConfigs must be moved as well. It seems checkPublishConfig can be (and should be) merged with modifyPublishConfig .

getResolvedPublishConfig must returns fulfilled effective publish config.

@develar develar self-requested a review March 20, 2017 15:47
@develar develar self-assigned this Mar 20, 2017
@develar
Copy link
Member

develar commented Mar 20, 2017

Thanks a lot for clear explanation. I will do mentioned fixes if you don't mind.

@markusdosch
Copy link
Contributor Author

I absolutely don't mind! 😊 Thanks for the fast response!

Clear explanations are the least I can do, in supporting this project! 🙂

@develar develar closed this in 55ebed1 Mar 21, 2017
@develar
Copy link
Member

develar commented Mar 21, 2017

@Sukram21 I will check perl error later.

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.

3 participants