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: introduce ci targets for lint/benchmark #5921

Closed

Conversation

jbergstroem
Copy link
Member

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?

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

Introduce two new targets we will populate with actions
once merged into all branches we need to support through CI.
@jbergstroem jbergstroem added build Issues and PRs related to build files or the CI. lts-watch-v4.x labels Mar 27, 2016
@jbergstroem
Copy link
Member Author

.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
Copy link
Contributor

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?

Copy link
Member Author

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?

@mscdex
Copy link
Contributor

mscdex commented Mar 27, 2016

Do we need to also reserve a jslint-ci target or no?

@jbergstroem
Copy link
Member Author

@mscdex said:

Do we need to also reserve a jslint-ci target or no?

I was thinking no since we could just do what we need to in lint-ci.

@mscdex
Copy link
Contributor

mscdex commented Mar 27, 2016

LGTM then

@jbergstroem
Copy link
Member Author

As long as we have {test,lint,benchmark}-ci I reckon we'll be fine. Worst case we'd have to copy paste if other places are updated. We even have run-ci (which I think we should move away from).

@MylesBorins
Copy link
Contributor

LGTM

@joaocgreis
Copy link
Member

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

LGTM

jasnell pushed a commit that referenced this pull request Apr 1, 2016
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>
@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

Landed in 8dcb82d

@jasnell jasnell closed this Apr 1, 2016
MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
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>
This was referenced Apr 5, 2016
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
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>
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
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