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: chmod binaries before publishing #43

Merged
merged 3 commits into from Apr 6, 2021

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Feb 1, 2021

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

@aminya aminya changed the title Chmod binaries before publishing fix: chmod binaries before publishing Feb 1, 2021
@aminya
Copy link
Contributor Author

aminya commented Feb 1, 2021

@nornagon This is a follow up to #42

@nornagon
Copy link
Member

nornagon commented Feb 1, 2021

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
Copy link
Contributor Author

aminya commented Feb 1, 2021

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
Member

nornagon commented Feb 1, 2021

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

build.js Outdated Show resolved Hide resolved
@aminya
Copy link
Contributor Author

aminya commented Feb 1, 2021

The issue is not fixed yet.

atom/atom#21916

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

I am on Windows (I publish from Windows). Not sure if that is relevant.

@aminya
Copy link
Contributor Author

aminya commented Apr 6, 2021

@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:master Apr 6, 2021
@nornagon
Copy link
Member

nornagon commented Apr 6, 2021

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

@aminya
Copy link
Contributor Author

aminya commented Apr 6, 2021

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

@nornagon
Copy link
Member

nornagon commented Apr 9, 2021

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
Copy link
Contributor Author

aminya commented Apr 9, 2021

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 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.

None yet

2 participants