-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: run-ci makefile rule #2134
Conversation
cc: @nodejs/build |
If |
Adding a single rule to be called from Jenkins. Jenkins jobs typically call: python ./configure make -j $(getconf _NPROCESSORS_ONLN) make test-ci After this change, we can have Jenkins call: make run-ci -j $(getconf _NPROCESSORS_ONLN) This allows us to customize how we call configure for different repos or branches (e.g. joyent\node).
119baa5
to
8195168
Compare
@rmg: agreed. Amended. |
LGTM 👍 |
Wouldn't |
The EDIT all:
@$(MAKE) foo
@$(MAKE) bar
foo bar:
@echo $@ $(MAKEFLAGS)
|
Can confirm above behaviour; just wanted to bring it up to make sure. Gmake is required as part of the build, so it should work out. |
Correct. From the make manual I gathered that it's better to let make manage that setting via MAKEFLAGS, so that it can control the total number of tasks running. |
LGTM |
@@ -185,6 +185,11 @@ docopen: out/doc/api/all.html | |||
docclean: | |||
-rm -rf out/doc | |||
|
|||
run-ci: | |||
$(PYTHON) ./configure $(CONFIG_FLAGS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are explicitly executing with python, do we still need to use ./
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not necessary but it doesn't hurt either, does it? We already invoke it like this in a couple other places in the makefile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, why not just ./configure $(CONFIG_FLAGS)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PYTHON is defined differently on different platforms. On some Jenkins slaves, this needs to be set explicitly via the environment variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thefourtheye I am assuming it's ok to land as is. Could you give an explicit +1 or -1? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orangemocha I am not sure if my vote counts, but LGTM as-is 👍
Adding a single rule to be called from Jenkins. Jenkins jobs typically call: python ./configure make -j $(getconf _NPROCESSORS_ONLN) make test-ci After this change, we can have Jenkins call: make run-ci -j $(getconf _NPROCESSORS_ONLN) This allows us to customize how we call configure for different repos or branches (e.g. joyent\node). PR-URL: #2134 Reviewed-By: Ryan Graham <[email protected]> Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Thank you! Landed in 12bc397 |
What would be the process for getting this merged into the next branch? |
@orangemocha IIRC, |
So that the Jenkins jobs can switch to using this rule, and cover the next branch as well. Thanks, I'll follow up on that. |
@orangemocha Ah, wait. I see that @trevnorris asked what will be merged to what, here, but the question was not clarified. |
Just need to merge latest release on master into next. (prefer release b/c it's less likely to have bugs that surface in next). Though eh, only one commit could possibly break. I'll do a merge after I get confirmation that it won't mess anyone else up. Will respond when I do. |
Great, thank you! |
Has landed on the next branch. |
Adding a single rule to be called from Jenkins. Jenkins jobs typically call: python ./configure make -j $(getconf _NPROCESSORS_ONLN) make test-ci After this change, we can have Jenkins call: make run-ci -j $(getconf _NPROCESSORS_ONLN) This allows us to customize how we call configure for different repos or branches (e.g. joyent\node). PR-URL: nodejs/node#2134 Reviewed-By: Ryan Graham <[email protected]> Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Adding a single rule to be called from Jenkins.
Jenkins jobs typically call:
python ./configure
make -j $(getconf _NPROCESSORS_ONLN)
make test-ci
After this change, we can have Jenkins call:
make run-ci -j $(getconf _NPROCESSORS_ONLN)
This allows us to customize how we call configure
for different repos or branches (e.g. joyent\node).