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

n-api: remove n-api module loading flag #14902

Closed
wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
Contributor

Remove the command line flag that was needed for N-API module loading.

Re: nodejs/vm#9

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

n-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Aug 17, 2017
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

FWIW

@refack
Copy link
Contributor

refack commented Aug 17, 2017

Isn't this semver-minor?

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

I have a couple questions. If they prove to be nothing, then LGTM.

@@ -3,4 +3,4 @@
import testpy

def GetConfiguration(context, root):
return testpy.AddonTestConfiguration(context, root, 'addons-napi', ['--napi-modules'])
return testpy.AddonTestConfiguration(context, root, 'addons-napi', [])
Copy link
Contributor

Choose a reason for hiding this comment

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

The [] can be dropped too I think.

if (!node::load_napi_modules) {
// NAPI is disabled, so set the module version to -1 to cause the module
// to be unloaded.
module_version = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you remove this line, where is the value set to -1?

src/node.cc Outdated
@@ -2537,27 +2534,20 @@ static void DLOpen(const FunctionCallbackInfo<Value>& args) {
env->ThrowError("Module did not self-register.");
return;
}
if (mp->nm_version != NODE_MODULE_VERSION) {
if (mp->nm_version == -1) {
ProcessEmitWarning(env, "N-API is an experimental feature "
Copy link
Contributor

Choose a reason for hiding this comment

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

Do any of our tests actually test for the warning?

src/node.cc Outdated
@@ -3912,8 +3900,6 @@ static void ParseArgs(int* argc,
force_repl = true;
} else if (strcmp(arg, "--no-deprecation") == 0) {
no_deprecation = true;
} else if (strcmp(arg, "--napi-modules") == 0) {
load_napi_modules = true;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe leave this in as a no-op for now?

Copy link
Member

Choose a reason for hiding this comment

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

I think that would be a good idea. It will give us time to update docs etc and will avoid breaking people who are already using it.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to leaving it for now.

@mhdawson
Copy link
Member

FYI @nodejs/ctc, @Fishrock123 to make people who were involved in earlier discussions comment/object in this PR if they still have issues with the approach.

src/node.cc Outdated
@@ -2537,27 +2534,20 @@ static void DLOpen(const FunctionCallbackInfo<Value>& args) {
env->ThrowError("Module did not self-register.");
return;
}
if (mp->nm_version != NODE_MODULE_VERSION) {
if (mp->nm_version == -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Do we want this warning to be printed on every n-api module load or just the first one? Complicates the code slightly but might be a better experience if n-api becomes more common?

Copy link
Member

Choose a reason for hiding this comment

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

typically we emit warnings only on the first use.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I would prefer this list to be smashed before this getting in a release.

LGTM

@gabrielschulhof
Copy link
Contributor Author

@mhdawson @digitalinfinity @addaleax @mcollina @cjihrig @refack I have now addressed the review comments.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

I'm ok with the changes but marking as "Request changes" so that we avoid landing until we have addressed this comment from @mcollina

I would prefer [this list](https://github.com/nodejs/abi-stable-node/issues/271) to be smashed before this getting in a release.

I think its reasonable that we either address or make the case why we don't need to in advance.

We plan to discuss the one related to CS modules with Bradley in our next regular N-API meeting (Thursday). @aruneshchandra since I think @jasongin is away can you chase down the status on the changes related to async hooks ?

@digitalinfinity
Copy link
Contributor

@mhdawson the n-api team chatted with @RReverser and @bmeck last Thursday about ES6 module supprt- the discussion is captured at nodejs/abi-stable-node#256 (comment). TLDR is that n-api will not attempt to provide first class support for any module loader, but we will be making a breaking change (@BoingBoing has already opened #15088)

@mhdawson
Copy link
Member

Discussed in latest TSC/CTC meeting there did not seem to be objections to removing once we get through the list of breaking changes. Was left that if any TSC/CTC members object they should come and discuss in this PR.

@BridgeAR
Copy link
Member

@mhdawson it seems like there are no objections anymore besides yours. I think think could land if you are good with it?

@mhdawson
Copy link
Member

@BridgeAR we are waiting until we have all of the remaining breaking changes in (we are close). We want those to land first as a group and then have this land as a second step.

@gabrielschulhof is in the weekly meeting were we are discussing, so is in the loop as to the next steps.

Once we are ready I'll go ahead and land this one.

@mhdawson
Copy link
Member

Looks like we are close to landing the remaining breaking changes, CI run:

https://ci.nodejs.org/job/node-test-pull-request/10081/

@mhdawson
Copy link
Member

mhdawson commented Sep 14, 2017

Now just waiting on #15108
@sampsongao will add the tests tonight and hopefully we can land Friday Sep 15
And then I'll follow that with this one.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM, just about to land last breaking change so this one can go in as well.

@mhdawson
Copy link
Member

@gabrielschulhof the tests no longer pass because of the changes to the Init signature. I'm done for today. Can you take a look and I'll try to land tomorrow.

Remove the command line flag that was needed for N-API module loading.

Re: nodejs/vm#9
@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

Weird linter failure, so I stopped that and started

https://ci.nodejs.org/job/node-test-pull-request/10142/

instead.

@gabrielschulhof
Copy link
Contributor Author

error: The last gc run reported the following. Please correct the root cause
and remove .git/gc.log.
Automatic cleanup will not be performed until the file is removed.

warning: There are too many unreachable loose objects; run 'git prune' to remove them.

@gabrielschulhof
Copy link
Contributor Author

I'll leave it running because make lint passes locally.

Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
Remove the command line flag that was needed for N-API module loading.
Re: nodejs/vm#9

PR-URL: nodejs/node#14902
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
Remove the command line flag that was needed for N-API module loading.
Re: nodejs/vm#9

PR-URL: nodejs/node#14902
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
@gabrielschulhof gabrielschulhof deleted the remove-flag branch September 25, 2017 09:20
jasnell added a commit that referenced this pull request Sep 25, 2017
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)
jasnell added a commit that referenced this pull request Sep 26, 2017
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
  * Custom lookup functions are now supported. [#14560](#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)
jasnell added a commit that referenced this pull request Sep 26, 2017
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
  * Custom lookup functions are now supported. [#14560](#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
  * Custom lookup functions are now supported. [#14560](#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
* **crypto**
  * Support for multiple ECDH curves. [#15206](nodejs/node#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](nodejs/node#7855)
  * Custom lookup functions are now supported. [#14560](nodejs/node#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](nodejs/node#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](nodejs/node#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](nodejs/node#15354)
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
Remove the command line flag that was needed for N-API module loading.
Re: nodejs/vm#9

PR-URL: nodejs#14902
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Remove the command line flag that was needed for N-API module loading.
Re: nodejs/vm#9

Backport-PR-URL: #19447
PR-URL: #14902
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
@richardlau richardlau mentioned this pull request Apr 25, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.