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

makefile: added option for http clone of gcovr #24073

Closed
wants to merge 2 commits into from
Closed

makefile: added option for http clone of gcovr #24073

wants to merge 2 commits into from

Conversation

mritunjayz
Copy link
Contributor

Checklist

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 4, 2018
@Trott
Copy link
Member

Trott commented Nov 4, 2018

@nodejs/build-files

@Trott
Copy link
Member

Trott commented Nov 4, 2018 •
edited
Loading

I don't think "try git protocol and then switch to https if it takes longer than 20 seconds" is a good approach. I also don't think that timeout is portable across all the platforms that run the Makefile such as macOS. The commit message needs to explain why this was added.

@Trott
Copy link
Member

Trott commented Nov 4, 2018

Applied this change, removed gcovr, and ran make coverage-build on macOS. Sure enough:

/bin/sh: timeout: command not found

It continued to run, which surprised me but makes sense now looking at the code.

@refack
Copy link
Contributor

refack commented Nov 4, 2018

I'd recommend simply switching the URI to https.

@Trott
Copy link
Member

Trott commented Nov 4, 2018

I'd recommend simply switching the URI to https.

Oh, yeah, that would at least make it consistent wherever git clone appears throughout the Makefile. @mritunjaygoutam12 What do you think?

@mritunjayz
Copy link
Contributor Author

I'd recommend simply switching the URI to https.

Oh, yeah, that would at least make it consistent wherever git clone appears throughout the Makefile. @mritunjaygoutam12 What do you think?

It will be fine to have one URI for git clone with http .
yeah, I missed it. Timeout is not portable for all platform.

@Trott
Copy link
Member

Trott commented Nov 4, 2018

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 4, 2018
@Trott
Copy link
Member

Trott commented Nov 8, 2018

Landed in 7cefc80

@Trott Trott closed this Nov 8, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 8, 2018
@mritunjayz @Trott
build: change repo to https protocol in Makefile
7cefc80 
PR-URL: nodejs#24073
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
@mritunjayz @BridgeAR
build: change repo to https protocol in Makefile
e4bcb97 
PR-URL: #24073
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BridgeAR BridgeAR mentioned this pull request Nov 14, 2018
Merged
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
@mritunjayz @kiyomizumia
build: change repo to https protocol in Makefile
0bb254c 
PR-URL: nodejs#24073
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
This was referenced Nov 15, 2018
Closed
Closed
Closed
Closed
Closed
Closed
codebytere pushed a commit that referenced this pull request Nov 29, 2018
@mritunjayz @codebytere
build: change repo to https protocol in Makefile
bda1cc7 
PR-URL: #24073
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@mritunjayz @MylesBorins
build: change repo to https protocol in Makefile
6936060 
PR-URL: #24073
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Dec 3, 2018
@mritunjayz @MylesBorins
build: change repo to https protocol in Makefile
95814f2 
PR-URL: #24073
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@Trott Trott Trott approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mritunjayz @Trott @refack @richardlau @codebytere @nodejs-github-bot