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: run-ci makefile rule #2134

Closed
wants to merge 1 commit into from

Conversation

orangemocha
Copy link
Contributor

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).

@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Jul 8, 2015
@orangemocha
Copy link
Contributor Author

cc: @nodejs/build

@rmg
Copy link
Contributor

rmg commented Jul 8, 2015

If configure-ci isn't going to be used directly, maybe roll it into run-ci so it's more obvious that run-ci is part of the "contract"?

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).
@orangemocha
Copy link
Contributor Author

@rmg: agreed. Amended.

@rmg
Copy link
Contributor

rmg commented Jul 8, 2015

LGTM 👍

@jbergstroem
Copy link
Member

Wouldn't -j have to be part of make?

@rmg
Copy link
Contributor

rmg commented Jul 8, 2015

The -j will be propagated automatically via $MAKEFLAGS.


EDIT
at least that's how GNU Make works.. I've never actually used another make..

all:
    @$(MAKE) foo
    @$(MAKE) bar
foo bar:
    @echo $@ $(MAKEFLAGS)
$ make
foo
bar
$ make -j 5
foo --jobserver-fds=3,4 -j
bar --jobserver-fds=3,4 -j

@jbergstroem
Copy link
Member

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.

@orangemocha
Copy link
Contributor Author

The -j will be propagated automatically via $MAKEFLAGS.

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.

@jbergstroem
Copy link
Member

LGTM

@@ -185,6 +185,11 @@ docopen: out/doc/api/all.html
docclean:
-rm -rf out/doc

run-ci:
$(PYTHON) ./configure $(CONFIG_FLAGS)
Copy link
Contributor

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 ./?

Copy link
Contributor Author

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.

Copy link
Contributor

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)?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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!

Copy link
Contributor

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 👍

orangemocha added a commit that referenced this pull request Jul 10, 2015
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 <r.m.graham@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@orangemocha
Copy link
Contributor Author

Thank you! Landed in 12bc397

@orangemocha
Copy link
Contributor Author

What would be the process for getting this merged into the next branch?

@thefourtheye
Copy link
Contributor

@orangemocha IIRC, next will be rebased with master, by 3.0.0 release. If you want to do it now, send in another PR targeting next. Why next branch?

@orangemocha
Copy link
Contributor Author

Why next branch?

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.

@thefourtheye
Copy link
Contributor

@orangemocha Ah, wait. I see that @trevnorris asked what will be merged to what, here, but the question was not clarified.

@trevnorris
Copy link
Contributor

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.

@orangemocha
Copy link
Contributor Author

Great, thank you!

@trevnorris
Copy link
Contributor

Has landed on the next branch.

orangemocha added a commit to orangemocha/node-1 that referenced this pull request Jul 30, 2015
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 <r.m.graham@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants