Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

feat: prebuild binaries for electron and node #67

Merged
merged 10 commits into from
Feb 28, 2018
Merged

Conversation

develar
Copy link
Contributor

@develar develar commented Jul 3, 2017

Closes #27

@develar develar mentioned this pull request Jul 3, 2017
@develar
Copy link
Contributor Author

develar commented Jul 3, 2017

You will need to add GH_TOKEN env (https://www.npmjs.com/package/prebuild#create-github-token) in Travis/Appveyor settings. I don't recommend to use encrypted env vars — better to set env using UI (links above to docs how to set). In case of AppVeyor, don't forget to click on lock icon to “Toggle variable encryption”.

prebuild upload command doesn't support setting GitHub token using env, so, special wrapper is added to be sure that token is not leaked in the CI server logs (Travis will hide it, but I am not sure about Appveyor).

@winstliu
Copy link
Contributor

What is the reason for switching from npm to yarn?

@develar
Copy link
Contributor Author

develar commented Jul 11, 2017

@50Wliu As maintainer of electron-builder, I am tired to advice users "remove node_modules and npm install again". Yarn fast and reliable. electron-userland/electron-builder#1610 (comment)

@popod
Copy link

popod commented Jul 20, 2017

@50Wliu will this PR merged in a near future ? I'm very interested in having prebuild binaries to generate my electron application easily. And thanks to develar for his work !

@develar
Copy link
Contributor Author

develar commented Jul 21, 2017

@popod for now you can use keytar-prebuild, but of course we need official release.

@popod
Copy link

popod commented Jul 21, 2017

@develar does keytar-prebuild works with latest electron version 1.7.5 ? and for future versions ? Do you need to do something for each versions or is it all automatic ?

Thanks for answer and sorry for the off topic..

@develar
Copy link
Contributor Author

develar commented Jul 21, 2017

@popod 1.7.5 doesn't introduce new ABI version (upcoming 1.8.0 does, but not yet released).

Do you need to do something for each versions or is it all automatic

Not automatic. Developer need to set version explicitly. Maybe automated, but... it occurs only ~ once a half year :)

@winstliu
Copy link
Contributor

@popod I can't say. I'm not familiar with this repository.

@popod
Copy link

popod commented Aug 9, 2017

When running unit tests with karma, I've this error:

ERROR in ./app/node_modules/keytar-prebuild/lib/keytar.js
Module not found: Error: Can't resolve 'node-loader' in '/rootPath'
 @ ./app/node_modules/keytar-prebuild/lib/keytar.js 1:13-52
 @ ./app/src/renderer/routes.js
 @ ./app/src/renderer ^\.\/(?!main(\.js)?$)
 @ ./test/unit/index.js

What should be the problem ?

@develar
Copy link
Contributor Author

develar commented Aug 9, 2017

@popod it seems in your project you use webpack and forget to add node-loader plugin.

@popod
Copy link

popod commented Aug 9, 2017

@develar Thanks.. I've installed it and this works.. but this is strange : without keytar, running the unit tests works well without installing node-loader.

@develar
Copy link
Contributor Author

develar commented Aug 9, 2017

@popod Nothing strange — keytar is a native module, so, there is .node file.

@develar
Copy link
Contributor Author

develar commented Sep 7, 2017

Rebased and Electron 1.8.0 support was added. I hope you will find time to make Electron developer's life more easier.

@develar
Copy link
Contributor Author

develar commented Sep 22, 2017

Ping :)

@develar
Copy link
Contributor Author

develar commented Sep 28, 2017

@zeke maybe you can review and accept this PR?

@zeke
Copy link

zeke commented Sep 28, 2017

I'm in support of the idea of using prebuild, but it would be the Atom team's call on whether to ship and maintain this.

None of the other Atom projects are using yarn, so I think sticking to npm would mean less disruption and give you better odds of getting this PR merged.

Also we unpublished electron 1.8.0 due to a security vulnerability, so this would need to be updated when 1.8.1 lands. (Shooting for a release of that very soon.)

cc @shiftkey, whom I know to be a user of keytar in https://github.com/desktop/desktop.

@develar
Copy link
Contributor Author

develar commented Sep 28, 2017

this would need to be updated when 1.8.1 lands.

It doesn't matter (because ABI version will be the same, if I am not wrong).

@Alex-D
Copy link

Alex-D commented Oct 19, 2017

What's up here?

Go npm if you want, but it will save a lof a time to get that merged :)

@zeke
Copy link

zeke commented Oct 19, 2017

I'm okay with landing this, assuming the requested yarn->npm changes are made to be consistent with the rest of the projects in the atom org.

@shiftkey
Copy link
Contributor

@zeke @develar apologies for the delay. I do want to have a look over this, but as I'm not responsible for ultimately publishing updates you might want to ensure that someone from the Atom team is 👍 .

@shiftkey shiftkey self-requested a review October 19, 2017 23:24
@kraenhansen
Copy link

It's a shame to see that this is held back because of a yarn vs npm debate.

@lee-dohm
Copy link
Contributor

👋 Thanks for the interest everyone.

With the holidays approaching, lots of efforts are being put on hold for the next couple weeks. Additionally, we're planning on transitioning and re-evaluating how the Atom team handles some things in January. Given all of that, it is unlikely that the Atom team will make a decision on whether to take this on or not until late January at the earliest.

It's on my list of things to bring up when things start getting picked up again in the new year. So if you have any questions at that point let me know and I'll see what I can do to get them answered.

@develar
Copy link
Contributor Author

develar commented Dec 14, 2017

As workaround, for now you can use fork keytar-prebuild.

@rongduan-zhu
Copy link

@develar Happy new year and huge thank you for providing prebuilt binaries for keytar! With the recent release of 4.1.0 there is a new function, findCredentials, that we would like to use. Can you please update the prebuild binaries to 4.1.0?

IGassmann added a commit to lbryio/lbry-desktop that referenced this pull request Jan 15, 2018
- Fix Electron linting errors.
- Add brew update to build script for getting latest dependencies versions.
- Clean .gitignore to remove IDE specific ignores and old rules.
- Add productName to build config.
- Replace dependency keytar with keytar-prebuild until atom/node-keytar#67 is not merged.
@daviwil daviwil self-assigned this Jan 19, 2018
@daviwil
Copy link
Contributor

daviwil commented Jan 19, 2018

Hey folks, Atom team member here. I'm going to help get this PR merged so we can have official prebuilds for keytar thanks to the work @develar has done so far! @develar, do you mind if I make some commits to your branch to simplify some things before I merge this?

@develar
Copy link
Contributor Author

develar commented Jan 20, 2018

@daviwil Branch updated, deps updated. Feel free to push to my branch.

@zeke
Copy link

zeke commented Jan 24, 2018

Thanks for jumping in on this, @daviwil. 👍

@daviwil
Copy link
Contributor

daviwil commented Jan 25, 2018

Tried this out yesterday, looks great! I'll make a couple tweaks so that it only does prebuilds for tags and then I'll get it merged today.

@daviwil
Copy link
Contributor

daviwil commented Jan 25, 2018

By the way, were the version bumps of babel-core, etc necessary, or was that Yarn's doing? I'd guess it was the latter.

@develar
Copy link
Contributor Author

develar commented Jan 25, 2018

I updated deps. To make explicit that tested on this dependency versions.

.npmignore Outdated
@@ -1,9 +0,0 @@
/build
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this file needs to be added back.

@daviwil
Copy link
Contributor

daviwil commented Feb 28, 2018

Thanks so much for putting this together @develar! We'll be providing prebuilds in the next release of node-keytar today or tomorrow :)

@daviwil daviwil merged commit ebca288 into atom:master Feb 28, 2018
@daviwil
Copy link
Contributor

daviwil commented Mar 1, 2018

v4.2.0 is out, please give it a shot and let me know if you run into any issues!

@zeke
Copy link

zeke commented Mar 1, 2018

Awesome! Thanks @develar and @daviwil for pushing this forward.

@IndieRobert
Copy link

IndieRobert commented Apr 16, 2018

Hi, Ive tried "npm install keytar-prebuild", works fine, but then I still have "Uncaught Error: A dynamic link library (DLL) initialization routine failed." error when I run my electron app using "npm start". Any idea?

os: windows 8.1 pro (x64)
electron: 1.8.4
node: v8.11.1

@vladimiry
Copy link

@IndieRobert why keytar-prebuild but not keytar? This project comes with prebuilds already.

@IndieRobert
Copy link

IndieRobert commented Apr 16, 2018

@vladimiry thx for helping. Tried that as well, didnt work, same error. (spec updated on previous comment)

@zeke
Copy link

zeke commented Apr 16, 2018

why keytar-prebuild but not keytar?

@develar created keytar-prebuild as a temporary fork. It should probably be deprecated to point people to the canonical module:

npm deprecate keytar-prebuild@"*" -m "keytar-prebuild is no longer maintained. Use `keytar` instead."

@develar develar deleted the prebuild branch April 17, 2018 08:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.