Skip to content

[v11.x-backport] Backport 24861 to v11.x - #25258

Closed
gabrielschulhof wants to merge 2 commits into
nodejs:v11.x-stagingfrom
gabrielschulhof:backport-24861-to-v11.x
Closed

[v11.x-backport] Backport 24861 to v11.x#25258
gabrielschulhof wants to merge 2 commits into
nodejs:v11.x-stagingfrom
gabrielschulhof:backport-24861-to-v11.x

Conversation

@gabrielschulhof

Copy link
Copy Markdown
Contributor
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

Re: #24861 (comment)

addaleaxand others added 2 commits December 28, 2018 11:30
Originally from portions of nodejs#23319.
PR-URL: nodejs#24861
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
This is an alternative to nodejs#23319
which attaches the loaded addons to the environment and closes them
when the environment is destroyed.
PR-URL: nodejs#24861
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v11.x labels Dec 28, 2018
@targos

Copy link
Copy Markdown
Member

What was the conflict?

@gabrielschulhof

Copy link
Copy Markdown
ContributorAuthor

@targos I didn't run into a conflict. I made this PR because there was a backport-requested tag. I was surprised to find no conflict. I figured although there might not be a source code conflict the tests might fail or something, but they didn't fail locally.

@gabrielschulhof

Copy link
Copy Markdown
ContributorAuthor

targos pushed a commit that referenced this pull request Jan 1, 2019
Originally from portions of #23319.
Backport-PR-URL: #25258
PR-URL: #24861
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Jan 1, 2019
This is an alternative to #23319
which attaches the loaded addons to the environment and closes them
when the environment is destroyed.
Backport-PR-URL: #25258
PR-URL: #24861
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos

Copy link
Copy Markdown
Member

Landed in 95353c7 and d718625. Thanks!

@targostargos closed this Jan 1, 2019
@MylesBorins

Copy link
Copy Markdown
Contributor

This needed to be tested with CITGM before it was landed. The original commits were breaking node-report

@MylesBorinsMylesBorins mentioned this pull request Jan 18, 2019
@addaleax

Copy link
Copy Markdown
Member

@MylesBorins I’m not sure, but maybe adding a backport request to a change that broke something in CITGM is a bit confusing? We still want a clean CITGM on master, so dont-land + a request to investigate could be a better approach for the future?

@gabrielschulhof
gabrielschulhof deleted the backport-24861-to-v11.x branch June 14, 2019 23:51
Sign up for freeto 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++.lib / srcIssues and PRs related to general changes in the lib or src directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@gabrielschulhof@nodejs-github-bot@targos@MylesBorins@addaleax