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

Finish Electron Support #271

Closed
wants to merge 20 commits into from
Closed

Finish Electron Support #271

wants to merge 20 commits into from

Conversation

adumbidiot
Copy link
Collaborator

@adumbidiot adumbidiot commented Nov 1, 2020

This PR will finish the work left from #270.

Jobs left:

  • The napi version feature is set by the executing nodejs, not the engine its targeting.
  • The bindgen generated headers are from the currently executing nodejs, not the target engine.
  • Add CI for electron on Windows, Linux, and Macos.

@adumbidiot
Copy link
Collaborator Author

adumbidiot commented Nov 1, 2020

I've been having some trouble with correctly setting the NAPI-feature. In order to determine the NAPI version, we need to build napi-sys and find the version in the headers. However, I don't think we can share the determined version to parent crates.

I also think that setting the NAPI version like this is kind of weird. Most users would build a library to target a certain minimum NAPI version. I think it would be better to move the napi features all the way to the top level in napi's toml, something like this:

[features]
napi7 = [ "napi6" ]
napi6 = [ "napi5" ]
napi5 = [ "napi4" ]
napi4 = [ "napi3" ]
napi3 = [ "napi2" ]
napi2 = [ "napi1" ]
napi1 = []

Then users of this library could choose a minimum NAPI version to support. If a user tried to compile with an napi version they don't support, they would get lots of compile errors as the low level sys functions they would need would not be present. Thoughts @Brooooooklyn?

While writing this I thought up a solution for scraping the NAPI version. We could have napi-build download and extract the archive, passing back the path to any crate that needs it. The crate could then use that path to either look for the NAPI version or create the bindings. napi-build will only be build once, so the headers will also only be downloaded once.

@Brooooooklyn
Copy link
Sponsor Member

Brooooooklyn commented Nov 1, 2020

Most users would build a library to target a certain minimum NAPI version.

Sounds reasonable.

[features]
napi7 = [ "napi6" ]
napi6 = [ "napi5" ]
napi5 = [ "napi4" ]
napi4 = [ "napi3" ]
napi3 = [ "napi2" ]
napi2 = [ "napi1" ]
napi1 = []

Looks nice.

@Brooooooklyn
Copy link
Sponsor Member

Rebase the latest master branch, and the bench action should work now.

@adumbidiot
Copy link
Collaborator Author

According to secrets setup on GitHub:
image

And according to GitHub Action Docs:
image

So, only collaborators can access secrets. GITHUB_TOKEN also cannot be used to comment. Maybe switch back to secrets.gh_token and disable that action if it is not present.

@adumbidiot
Copy link
Collaborator Author

I need to find a good way to send the NAPI version info through the commands on CI now. I will sleep on it.

latin1 = ["encoding_rs"]
libuv = ["futures"]
serde-json = ["serde", "serde_json"]
tokio_rt = ["futures", "tokio", "once_cell"]

napi7 = [ "napi6" ]
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

what if add a feature system-napi to compatible with the previous behavior? So that you don't need to change the build scripts on CI and people have more choice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to avoid that that since thats probably not what most people want, but I agree that it would simplify everything greatly and provide backwards compatibility. I'll make that change and document these features on the readme.

@adumbidiot
Copy link
Collaborator Author

Closing as adding an napi feature will be much easier after #290 lands. Downloading headers will also become necessary, invaliding all work done here.

@adumbidiot adumbidiot closed this Nov 10, 2020
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