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

Update binding.gyp to allow compiling against Electron 20+ headers #44

Conversation

carterbs
Copy link

@carterbs carterbs commented Oct 28, 2022

Currently, I cannot install native-keymap with electron-builder when targeting [email protected].

Electron builder compiles native-keymap against the chromium headers for the target version. In Electron 20+, they started using C++17 syntax, and native-keymap cannot compile against those headers. This PR changes binding.gyp so that native-keymap will compile against Electron 21's headers.

This addresses the issue described here: #43 (comment)

Note: I'm not a cpp developer and I'm not 100% sure of the ramifications of changing the target. It compiles and works in electron quick-start though, so 🤞.

@deepak1556
Copy link
Contributor

Thanks for the PR! The requirement to use newer C++ version comes from the runtime (v8 headers shipped with newer versions of Electron) which means the necessary compiler flags should be set in the gypi files shipped by the runtime as well. The module itself does not use any C++17 features and the module is meant to be compiled for node runtime as well as electron. I am -1 on adding this change to the module, I will see if the issue can be addressed in Electron core, please prefer to use electron-rebuild in the meantime.

@deepak1556 deepak1556 closed this Oct 29, 2022
@carterbs
Copy link
Author

I'll give it a go. Thanks!

@PF4Public
Copy link

@deepak1556 Have you addressed this issue in Electron? I could not find a PR for that.

@deepak1556
Copy link
Contributor

Yes it is addressed in electron/electron#36369, will be available in releases this week.

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