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

modules: adding load linked modules feature #8

Closed
wants to merge 1 commit into from
Closed

modules: adding load linked modules feature #8

wants to merge 1 commit into from

Conversation

thlorenz
Copy link
Contributor

refiling PR from node-fwd

  • introduced NM_F_LINKED flag to identify linked modules
  • setting node_is_initialized after calling V8::Initialize in order to
    make the right decision during initial module registration
  • introduced modlist_linked in order to track modules that were
    pre-registered in order to complete it once node is initialized
  • completing registration of linked module similarly to the way it's
    done inside DLOpen

- introduced NM_F_LINKED flag to identify linked modules
- setting node_is_initialized after calling V8::Initialize in order to
  make the right decision during initial module registration
- introduced modlist_linked in order to track modules that were
  pre-registered in order to complete it once node is initialized
- completing registration of linked module similarly to the way it's
  done inside DLOpen
@bnoordhuis
Copy link
Member

For the record, I already LGTM'd this but I'd like @trevnorris to sign off on it.

@trevnorris
Copy link
Contributor

LGTM

@rvagg
Copy link
Member

rvagg commented Dec 1, 2014

@rvagg
Copy link
Member

rvagg commented Dec 1, 2014

@thlorenz would it be difficult to add a simple test for this in test/addons/ and perhaps a super-simple sanity check in test/simple/? seems like something that's ripe for regressions.

@thlorenz
Copy link
Contributor Author

thlorenz commented Dec 1, 2014

Ideally yes, but that would require adding something to node.gyp the way nad does.
That would also mean we'd have to recompile to run it.

So I think it'd have to be some integration test, possibly even executed manually, not sure.

Not sure when I can get to it. Could we merge this anyhow and I'll think about how to best do this and supply tests in another PR?

@trevnorris
Copy link
Contributor

I'd normally require a test, but since the API is technically "private" (done, IMO, so the API can be hashed out as it's used if necessary) there's no need for a test at the moment.

@thlorenz
Copy link
Contributor Author

thlorenz commented Dec 3, 2014

Thanks @trevnorris and as I said it won't be a simple test.
Will hash out with @rvagg how to best include and run these integrationy tests in general and add one for this at that point.

@rvagg
Copy link
Member

rvagg commented Dec 3, 2014

@thlorenz I assumed that the test/addons/ directory would be the place for this--they are a separate test run and are all compiled and loaded to test, perhaps we could do something similar to make this testable?

@thlorenz
Copy link
Contributor Author

thlorenz commented Dec 3, 2014

No this doesn't work exactly like an addon. It's a linked module, i.e. it has to be made part of the code base by modifying the node.gyp file (what nad does).

@rvagg
Copy link
Member

rvagg commented Dec 4, 2014

this got landed in joyent/node didn't it? should we land it here or close and wait for the next pull from joyent/node?

@thlorenz
Copy link
Contributor Author

thlorenz commented Dec 4, 2014

It did land in joyent/node as well but in different shape since things were adapted to older API, i.e. env->SET_MODULE was not available there.

rvagg pushed a commit that referenced this pull request Dec 4, 2014
- introduced NM_F_LINKED flag to identify linked modules
- setting node_is_initialized after calling V8::Initialize in order to
  make the right decision during initial module registration
- introduced modlist_linked in order to track modules that were
  pre-registered in order to complete it once node is initialized
- completing registration of linked module similarly to the way it's
  done inside DLOpen

PR-URL: #8
Reviewed-by: Trevor Norris <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@rvagg
Copy link
Member

rvagg commented Dec 4, 2014

landed via f4e058d

@rvagg rvagg closed this Dec 4, 2014
rvagg pushed a commit that referenced this pull request Dec 4, 2014
- introduced NM_F_LINKED flag to identify linked modules
- setting node_is_initialized after calling V8::Initialize in order to
  make the right decision during initial module registration
- introduced modlist_linked in order to track modules that were
  pre-registered in order to complete it once node is initialized
- completing registration of linked module similarly to the way it's
  done inside DLOpen

PR-URL: #8
Reviewed-by: Trevor Norris <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 4, 2014
- introduced NM_F_LINKED flag to identify linked modules
- setting node_is_initialized after calling V8::Initialize in order to
  make the right decision during initial module registration
- introduced modlist_linked in order to track modules that were
  pre-registered in order to complete it once node is initialized
- completing registration of linked module similarly to the way it's
  done inside DLOpen

PR-URL: #8
Reviewed-by: Trevor Norris <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 4, 2014
- introduced NM_F_LINKED flag to identify linked modules
- setting node_is_initialized after calling V8::Initialize in order to
  make the right decision during initial module registration
- introduced modlist_linked in order to track modules that were
  pre-registered in order to complete it once node is initialized
- completing registration of linked module similarly to the way it's
  done inside DLOpen

PR-URL: #8
Reviewed-by: Trevor Norris <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@dmitryame dmitryame mentioned this pull request Mar 10, 2016
@rdkgit rdkgit mentioned this pull request Nov 11, 2016
@myitcv myitcv mentioned this pull request Aug 1, 2017
targos referenced this pull request in targos/node Apr 17, 2021
Original commit message:

    Merged: [compiler] Fix bug in SimplifiedLowering's overflow computation

    Revision: e371325bcb03f20a362ebfa48225159702c6fde7

    BUG=chromium:1126249
    NOTRY=true
    NOPRESUBMIT=true
    NOTREECHECKS=true
    [email protected]

    Change-Id: I411d9233f77992e73da12784cef59c885999b556
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2415988
    Reviewed-by: Tobias Tebbi <[email protected]>
    Commit-Queue: Georg Neis <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{#8}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@a59e3ac
targos referenced this pull request in targos/node Apr 27, 2021
Original commit message:

    Merged: [compiler] Fix bug in SimplifiedLowering's overflow computation

    Revision: e371325bcb03f20a362ebfa48225159702c6fde7

    BUG=chromium:1126249
    NOTRY=true
    NOPRESUBMIT=true
    NOTREECHECKS=true
    [email protected]

    Change-Id: I411d9233f77992e73da12784cef59c885999b556
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2415988
    Reviewed-by: Tobias Tebbi <[email protected]>
    Commit-Queue: Georg Neis <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{#8}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@a59e3ac
targos added a commit that referenced this pull request Apr 30, 2021
Original commit message:

    Merged: [compiler] Fix bug in SimplifiedLowering's overflow computation

    Revision: e371325bcb03f20a362ebfa48225159702c6fde7

    BUG=chromium:1126249
    NOTRY=true
    NOPRESUBMIT=true
    NOTREECHECKS=true
    [email protected]

    Change-Id: I411d9233f77992e73da12784cef59c885999b556
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2415988
    Reviewed-by: Tobias Tebbi <[email protected]>
    Commit-Queue: Georg Neis <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{#8}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@a59e3ac

PR-URL: #38275
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
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.

4 participants