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.

Sign up for GitHub

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

Jump to bottom

[v4.x backport] Use external headers in addons tests #16550

Closed
wants to merge 1 commit into from
Closed

[v4.x backport] Use external headers in addons tests #16550

wants to merge 1 commit into from

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Oct 27, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test/addons

@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. test Issues and PRs related to the tests. v4.x labels Oct 27, 2017
@gibfahn
Copy link
Member Author

gibfahn commented Oct 27, 2017 •
edited
Loading

Just backporting one commit (3c85f4e) from #6734, and #7947. Using internal headers like include <util.h> breaks our (IBM) CI, so it'd be nice to fix this in the next 4.x release. Should be low risk as it only modifies tests.

@MylesBorins Given that the rc is already out, if you don't want to pick this up then I understand.

This cherry-picked cleanly (except for files that don't exist on 4.x).

@gibfahn
Copy link
Member Author

gibfahn commented Oct 27, 2017

CI: https://ci.nodejs.org/job/node-test-commit/13509/

@bnoordhuis
Copy link
Member

@gibfahn Can you also update test/addons/openssl-binding/binding.cc?

@gibfahn
Copy link
Member Author

gibfahn commented Oct 27, 2017

@gibfahn Can you also update test/addons/openssl-binding/binding.cc?

Added #7947

@gibfahn
Copy link
Member Author

gibfahn commented Oct 27, 2017 •
edited
Loading

CI 2: https://ci.nodejs.org/job/node-test-commit/13521/

MylesBorins pushed a commit that referenced this pull request Nov 2, 2017
@gibfahn @MylesBorins
test: remove internal headers from addons
cb92f93 
Backport-PR-URL: #16550
PR-URL: #7947
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

landed in cb92f93

@MylesBorins MylesBorins closed this Nov 2, 2017
@gibfahn gibfahn deleted the ext-headers-v4.x branch November 2, 2017 23:09
@gibfahn gibfahn restored the ext-headers-v4.x branch November 28, 2017 15:12
@gibfahn gibfahn reopened this Nov 28, 2017
@gibfahn
Copy link
Member Author

gibfahn commented Nov 28, 2017

@MylesBorins I think you only landed one of the two commits here, could you include @bnoordhuis 's commit as well? I'll rebase on v4.x-staging.

@bnoordhuis @gibfahn
test: don't use internal headers in add-on tests
b520c67 
There is no real need and it causes endless grief on Windows with some
of the upcoming changes.

PR-URL: nodejs#6734
Backport-PR-URL: nodejs#16550
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@gibfahn
Copy link
Member Author

gibfahn commented Nov 28, 2017

Rebased and removed the commit that already landed, @MylesBorins PTAL.

MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@bnoordhuis @MylesBorins
test: don't use internal headers in add-on tests
b19c123 
There is no real need and it causes endless grief on Windows with some
of the upcoming changes.

PR-URL: #6734
Backport-PR-URL: #16550
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

aw shucks,

second commit landed in b19c123

@gibfahn gibfahn deleted the ext-headers-v4.x branch November 28, 2017 16:36
MylesBorins pushed a commit that referenced this pull request Dec 7, 2017
@bnoordhuis @MylesBorins
test: don't use internal headers in add-on tests
361aa01 
There is no real need and it causes endless grief on Windows with some
of the upcoming changes.

PR-URL: #6734
Backport-PR-URL: #16550
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 8, 2017
@bnoordhuis @MylesBorins
test: don't use internal headers in add-on tests
ed7bfb5 
There is no real need and it causes endless grief on Windows with some
of the upcoming changes.

PR-URL: #6734
Backport-PR-URL: #16550
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@bnoordhuis @MylesBorins
test: don't use internal headers in add-on tests
7a05cd3 
There is no real need and it causes endless grief on Windows with some
of the upcoming changes.

PR-URL: #6734
Backport-PR-URL: #16550
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
@bnoordhuis @MylesBorins
test: don't use internal headers in add-on tests
a65ca3f 
There is no real need and it causes endless grief on Windows with some
of the upcoming changes.

PR-URL: #6734
Backport-PR-URL: #16550
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@MylesBorins MylesBorins Awaiting requested review from MylesBorins

Assignees
No one assigned
Labels
addons Issues and PRs related to native addons. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gibfahn @bnoordhuis @MylesBorins @nodejs-github-bot