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: use existing variable to reduce complexity #2883

Closed
wants to merge 1 commit into from
Closed

Makefile: use existing variable to reduce complexity #2883

wants to merge 1 commit into from

Conversation

brycebaril
Copy link
Contributor

cc @rvagg

@Fishrock123 Fishrock123 added the build Issues and PRs related to build files or the CI. label Sep 15, 2015
@rvagg
Copy link
Member

rvagg commented Sep 16, 2015

I might use a nightly to test this, it looks good but hard to test locally

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

@brycebaril @rvagg ... ping

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 22, 2016
@jbergstroem
Copy link
Member

If no one else wants to handle this I'm happy to resume. Generally looks good to me; just need to test a bit.

@Fishrock123
Copy link
Contributor

@jbergstroem I think the silence is a go-ahead

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01
@jasnell
Copy link
Member

jasnell commented Feb 28, 2017

ping @nodejs/build

@jasnell
Copy link
Member

jasnell commented Mar 24, 2017

ping @jbergstroem

@fhinkel
Copy link
Member

fhinkel commented May 23, 2017

There hasn't been any progress here for quite a while. I'm going ahead and closing this. Feel free to re-open or ping a collaborator if you want it to stay open.

@fhinkel fhinkel closed this May 23, 2017
@gibfahn gibfahn reopened this May 23, 2017
@gibfahn
Copy link
Member

gibfahn commented May 23, 2017

Thanks for the prod @fhinkel !

This seems like a clear improvement, and should be really easy to review. @nodejs/build @nodejs/release can anyone else take a look?

@brycebaril sorry this got lost.

Amazingly this still patches cleanly (with git am -3), I guess the Makefile hasn't changed much.

addaleax
addaleax approved these changes May 23, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t see any reason not to land this today :D

gibfahn
gibfahn approved these changes May 23, 2017
View reviewed changes
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given TARNAME=node-$(FULLVERSION) is set earlier, this seems pretty clearly okay.

@gibfahn gibfahn self-assigned this May 23, 2017
@gibfahn gibfahn removed the stalled Issues and PRs that are stalled. label May 23, 2017
@fhinkel
Copy link
Member

fhinkel commented Jun 7, 2017

Thanks. Landed in ca50a19

@fhinkel fhinkel closed this Jun 7, 2017
fhinkel pushed a commit that referenced this pull request Jun 7, 2017
@brycebaril @fhinkel
build: use existing variable to reduce complexity
ca50a19 
PR-URL: #2883
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
jasnell pushed a commit that referenced this pull request Jun 7, 2017
@brycebaril @jasnell
build: use existing variable to reduce complexity
65f22e4 
PR-URL: #2883
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
MylesBorins pushed a commit that referenced this pull request Jul 17, 2017
@brycebaril @MylesBorins
build: use existing variable to reduce complexity
b0c6bf8 
PR-URL: #2883
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sam-github sam-github sam-github approved these changes

@fhinkel fhinkel fhinkel approved these changes

@addaleax addaleax addaleax approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees

@gibfahn gibfahn

Labels
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.
10 participants
@brycebaril @rvagg @jasnell @jbergstroem @Fishrock123 @fhinkel @gibfahn @sam-github @addaleax @MylesBorins