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

build: don't fail make test on source tarballs #15441

Closed
wants to merge 1 commit into from

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Sep 16, 2017

Tries to achieve the same effect as #13658 without breaking source tarballs. Presumably if tools/eslint wasn't there at all, people would notice in the PR review!

Fixes: #14513

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

build

Tries to achieve the same effect as
nodejs#13658 without breaking source
tarballs. Presumably if `tools/eslint` wasn't there at all, people
would notice in the PR review!

Fixes: nodejs#14513
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Sep 16, 2017
@gibfahn
Copy link
Member Author

gibfahn commented Sep 16, 2017

cc/ @jellelicht @vsemozhetbyt @joaocgreis, is this a reasonable compromise?

@gibfahn gibfahn self-assigned this Sep 16, 2017
@jellelicht
Copy link

I would be extremely happy if this is merged 👍

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

@BridgeAR
Copy link
Member

Landed in 66e45b8

@BridgeAR BridgeAR closed this Sep 21, 2017
BridgeAR pushed a commit that referenced this pull request Sep 21, 2017
Tries to achieve the same effect as
#13658 without breaking source
tarballs. Presumably if `tools/eslint` wasn't there at all, people
would notice in the PR review!

PR-URL: #15441
Fixes: #14513
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn gibfahn deleted the src-tarballs branch September 22, 2017 12:59
@gibfahn
Copy link
Member Author

gibfahn commented Sep 22, 2017

This should be backported to 6.x, as it's essentially a fix for people using make test on source tarballs who were broken by #13658.

@gibfahn
Copy link
Member Author

gibfahn commented Sep 22, 2017

Also @BridgeAR this landed without CI.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 22, 2017

@gibfahn right, that slipped through. I make sure it will run next time.

lukaszewczak pushed a commit to lukaszewczak/node that referenced this pull request Sep 23, 2017
Tries to achieve the same effect as
nodejs#13658 without breaking source
tarballs. Presumably if `tools/eslint` wasn't there at all, people
would notice in the PR review!

PR-URL: nodejs#15441
Fixes: nodejs#14513
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 23, 2017
Tries to achieve the same effect as
nodejs/node#13658 without breaking source
tarballs. Presumably if `tools/eslint` wasn't there at all, people
would notice in the PR review!

PR-URL: nodejs/node#15441
Fixes: nodejs/node#14513
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 25, 2017
Tries to achieve the same effect as
#13658 without breaking source
tarballs. Presumably if `tools/eslint` wasn't there at all, people
would notice in the PR review!

PR-URL: #15441
Fixes: #14513
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 17, 2017
Tries to achieve the same effect as
#13658 without breaking source
tarballs. Presumably if `tools/eslint` wasn't there at all, people
would notice in the PR review!

PR-URL: #15441
Fixes: #14513
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

cherry-picked cleanly @gibfahn

@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
Tries to achieve the same effect as
#13658 without breaking source
tarballs. Presumably if `tools/eslint` wasn't there at all, people
would notice in the PR review!

PR-URL: #15441
Fixes: #14513
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants