-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: introduce ci targets for lint/benchmark #5921
build: introduce ci targets for lint/benchmark #5921
Conversation
Introduce two new targets we will populate with actions once merged into all branches we need to support through CI.
.PHONY: lint cpplint jslint bench clean docopen docclean doc dist distclean \ | ||
check uninstall install install-includes install-bin all staticlib \ | ||
dynamiclib test test-all test-addons build-addons website-upload pkg \ | ||
blog blogclean tar binary release-only bench-http-simple bench-idle \ | ||
bench-all bench bench-misc bench-array bench-buffer bench-net \ | ||
bench-http bench-fs bench-tls cctest run-ci test-v8 test-v8-intl \ | ||
test-v8-benchmarks test-v8-all v8 | ||
test-v8-benchmarks test-v8-all v8 lint-ci |
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.
Can we remove lint
at the beginning of this then?
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.
no, its still a target?
Do we need to also reserve a |
@mscdex said:
I was thinking no since we could just do what we need to in |
LGTM then |
As long as we have |
LGTM |
LGTM |
1 similar comment
LGTM |
Introduce two new targets we will populate with actions once merged into all branches we need to support through CI. PR-URL: #5921 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Landed in 8dcb82d |
Introduce two new targets we will populate with actions once merged into all branches we need to support through CI. PR-URL: #5921 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Introduce two new targets we will populate with actions once merged into all branches we need to support through CI. PR-URL: #5921 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Pull Request check-list
make -j8 test
(UNIX) orvcbuild test nosign
(Windows) pass withthis change (including linting)?
Affected core subsystem(s)
build
Description of change
Introduce two new targets we will populate with actions once merged into all branches we need to support through CI. Refs #5638.
The gist of the problem is that we can't call makefile targets through CI that doesn't exist in all branches we test on. Meaning, the quicker we get it in, the easier it will be to expand on changes to linting, testing and benchmarking (making sure we call respective
-ci
targets from CI).vcbuild.bat
changes are missing, but linting and benchmarking needs more improvements than just ci - and we don't invoke linting/benchmarking stuff from Windows just yet./cc @nodejs/build, @mscdex