This repository has been archived by the owner on Mar 4, 2020. It is now read-only.
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: use new NMV of 69. Refs: nodejs/TSC#651
- Loading branch information
1 parent
4d44266
commit 8bc5d17
8bc5d17There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarshallOfSound Please explain, where did you get this number. There is no Node 69, the latest version is 67. Because of this change in 4.0.4 native modules build fails using electron-builder, on 4.0.3 everything is fine. Electron is running with Node 10.11 which is 64. The Native modules are build for the correct node, but they don't work anymore because of this 69.
8bc5d17There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@talamaska The commit message here has a link to the full thread and reasoning. Please note this is the correct thing to do, you just need to rebuild your modules for Electron 4.0.4 and higher.
8bc5d17There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarshallOfSound I don't get it. You are inventing your own version that is not used by Node itself for not understable reason for me as a user of electron, I see that sqllite is failing and I need to use a different way to build the native module for this version of electron. So who is the problem here. running npm rebuild break the sqlite dependency, but running electron-rebuild works fine. So how you plan on fixing this. Since most of the apps are using electron-builder as dependency to facilitate building process without need to understand and know everything about node-gyp and electron-gyp. Should the maintainers of electron-userland do something?
8bc5d17There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarshallOfSound Plus why you make this breaking change in the middle of nowhere with a minor version release?
8bc5d17There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@talamaska Before we continue this conversation I'd just like to remind you of our Code of Conduct and that everyone involved here is acting in good faith and for good reasons, please keep that in mind when deciding how to communicate with us.
In answer to your questions:
No we didn't invent it, as I linked above this went through the proper process for getting a custom ABI number up-to and including having a discussion with the NodeJS TSC. We didn't just make this number up, we deliberately requested an un-used ABI number as Electron 4 is not ABI compatible with the equivalent version of node.
Right now, nothing. Notihng is broken, if a third party solution (builder in this case) has a flaw in it's rebuilding logic that is the responsibility of that package, not this team. Our rebuilding package
electron-rebuild(as you have noted yourself) works, there is no issue with the switch itself, just another tools rebuilding logic.Possible, I am unfamiliar with how
electron-builderdoes its native module rebuilding.It is noted on their own tracker though that
electron-rebuild(the "official" solution for rebuilding native modules for electron) works for this version bump. You may want to follow along or ask questions in --> electron-userland/electron-builder#3660This wasn't technically classified as a breaking change, it was a "fix", until Electron 4.0.4 the ABI number in Electron 4 was incorrect. This was a fix, not a breaking change.
8bc5d17There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I sounded rude, I'm not, but I was frustrated, since suddenly I can't build my app. And if you notice I did my research in their issue tracker to find a solution, which according to some people is to move back to 4.0.3 instead, which I don't want since the newer versions has fixes. It would be nice to help them out with fixing this issue.
But you can't expect users of electron to follow all the liines of your conversations with node foundation, know what is ABI etc.
As I remember Electron's idea is to use your front-end/node.js skills to create desktop apps and knowing the internals is not something that everybody has time to invest in learning and fixing.
Thanks for all your answers.
8bc5d17There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really is a breaking change for precompiled modules. Because of the semantic versioning contracts, tools like node-pre-gyp will consider with good reasons that versions 4.0.3 and 4.0.4 to have the same ABI.
See grpc/grpc-node#762 for an example of existing package breaking due to this.
And I don't know how to properly solve this conundrum. Either break people who update their version of electron to 4.0.4, or break people who don't.
8bc5d17There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on this is a breaking change, see another example greenheartgames/greenworks#219.
8bc5d17There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarshallOfSound a fix is a breaking change. And you plan to unfix this in another minor version.
I suggest releasing the second fix as
electron 5.0.0to avoid more issues.I think the native ABI should not change in the patch or minor versions, just major versions.
8bc5d17There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So now I have to compile all my binary modules like GRPC (see above), zeromq (see https://github.com/zeromq/zeromq.js/releases - also provides precompiled binaries for ABI 64 not 69) and several more myself to have a working application using Electron >=4.0.4 (which is actually the latest non beta version of electron that provides the latest supported node LTS version)?
Is it 100% ensured that there will never be a official node ABI 69?
8bc5d17There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this causes also issues with NAN 2.13.0, see nodejs/nan#839