Skip to content

fix: chmod binaries before publishing - #43

Merged
nornagon merged 3 commits into
electron:masterfrom
aminya:chmod-before-publish
Apr 6, 2021
Merged

fix: chmod binaries before publishing#43
nornagon merged 3 commits into
electron:masterfrom
aminya:chmod-before-publish

Conversation

@aminya

@aminyaaminya commented Feb 1, 2021

Copy link
Copy Markdown
Contributor

The permissions might be lost when uploaded and downloaded again. This ensures they are executables before publishing.

The reason: https://github.com/actions/upload-artifact#permission-loss

@aminyaaminya changed the title Chmod binaries before publishingfix: chmod binaries before publishingFeb 1, 2021
@aminya

Copy link
Copy Markdown
ContributorAuthor

@nornagon This is a follow up to #42

@nornagon

Copy link
Copy Markdown
Collaborator

Have you observed a failure that this fixes? As far as I can tell, the executables should have the correct permissions after being built, copyFileSync preserves permissions, and npm packages are tarballs, which preserve permissions, so this shouldn't be necessary.

@aminya

aminya commented Feb 1, 2021

Copy link
Copy Markdown
ContributorAuthor

In the Atom CI, there is an EACCES issue which I think is fixed by this.

https://github.visualstudio.com/Atom/_build/results?buildId=97695&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=274baec0-c438-5f9e-e2fa-e6a84e183e1e&l=34

I am running a new CI on the new version of my fork which includes this PR:
atom/atom#21916

@nornagon

Copy link
Copy Markdown
Collaborator

Ah, I see, the permission is lost between Github Actions and npm publish.

Comment threadbuild.js Outdated
@aminya

aminya commented Feb 1, 2021

Copy link
Copy Markdown
ContributorAuthor

@aminya
aminyaforce-pushed the chmod-before-publish branch from fb55d92 to 09f473bCompareApril 6, 2021 21:45
@aminya

aminya commented Apr 6, 2021

Copy link
Copy Markdown
ContributorAuthor

@nornagon Could you merge this, pack the package and test it in a dummy folder? Because I am on Windows, I can't easily verify if the permissions change on Linux binaries.

@nornagon
nornagon merged commit 0887be6 into electron:masterApr 6, 2021
@nornagon

Copy link
Copy Markdown
Collaborator

@aminya publishing should happen automatically, you can test it yourself after that I believe!

@aminya

Copy link
Copy Markdown
ContributorAuthor

It's about an hour and nothing has been published. Are you sure that the publishing is automatic?

@nornagon

nornagon commented Apr 9, 2021

Copy link
Copy Markdown
Collaborator

Ah, it looks like indeed this repo isn't set up for that. My apologies, most in the Electron organization are.

On closer inspection though, it seems this PR will have no effect, because the published packages do not contain anything in bin/: https://unpkg.com/browse/minidump@0.19.0/

[EDIT]: ah I see, we've updated the build instructions since then. Sorry, it's been a minute and I'm operating at new-dad brain levels 😅

@aminya

aminya commented Apr 9, 2021

Copy link
Copy Markdown
ContributorAuthor

Yes, I made this PR as a follow-up to #42 which was never published too.

Would you please register a new version manually? It will fix #46.

Sign up for freeto 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

@aminya@nornagon