Skip to content
This repository was archived by the owner on Aug 31, 2018. It is now read-only.

ci: run the linter from travis with every build #75

Closed
Fishrock123 wants to merge 1 commit into from
Closed

ci: run the linter from travis with every build #75

Fishrock123 wants to merge 1 commit into from

Conversation

@Fishrock123
Copy link
Contributor

@Fishrock123 Fishrock123 commented Sep 20, 2017 •
edited
Loading

It'd be nice to have it run as a separate task, but I think this should
work for now.

Refs: #71

cc @addaleax

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

ci

ljharb
ljharb reviewed Sep 20, 2017
View reviewed changes
.travis.yml Outdated
- make -j2 V=
script:
- make -j2 test-ci
- make lint
Copy link

@ljharb ljharb Sep 20, 2017

Choose a reason for hiding this comment

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

You can make it a separate task with include, an env var, and a conditional in script - it's probably worth doing it now.

@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Sep 21, 2017

Oops, looks like the linter is still trying to build.

I really do not find travis' docs very usable. :/

ljharb
ljharb reviewed Sep 21, 2017
View reviewed changes
tools/run-ci.sh Outdated
# always change the working directory to the project's root directory
cd $(dirname $0)/..

if [ "${LINTER_ONLY}" = "true" ]; then
Copy link

@ljharb ljharb Sep 21, 2017

Choose a reason for hiding this comment

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

you may want ${LINTER_ONLY-} just in case the u option is set

Copy link
Contributor Author

@Fishrock123 Fishrock123 Sep 21, 2017

Choose a reason for hiding this comment

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

I'm not sure what you mean - in case set -u is used in the bash script?

@sandfox
Copy link

sandfox commented Sep 21, 2017

Is the intention to have make make lint and make test run as separate (and therefore, parallelisable/concurrent as separate tasks)?

@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Sep 21, 2017

Is the intention to have make make lint and make test run as separate (and therefore, parallelisable/concurrent as separate tasks)?

Correct. This is how it is in the Node CI, e.g. https://ci.nodejs.org/job/node-test-commit/12497/.
It is helpful to have the linter results show separately.

@Fishrock123 Fishrock123 force-pushed the ayo-linter branch 2 times, most recently from 147396f to c5ff6fc Compare September 21, 2017 15:39
Fishrock123 added a commit to Fishrock123/ayo that referenced this pull request Sep 21, 2017
@Fishrock123
ci: bah, just move everything into matrix:
c5ff6fc 
PR-URL: ayojs#75
Fishrock123 added a commit to Fishrock123/ayo that referenced this pull request Sep 21, 2017
@Fishrock123
ci: bah, just move everything into matrix:
7dc4f34 
PR-URL: ayojs#75
Fishrock123 added a commit to Fishrock123/ayo that referenced this pull request Sep 21, 2017
@Fishrock123
ci: bah, just move everything into matrix:
811cdb3 
PR-URL: ayojs#75
@TimothyGu
Copy link
Member

TimothyGu commented Sep 21, 2017

Where is run-ci.sh used?

@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Sep 21, 2017

@TimothyGu Please check the commit history. I have no way of testing this locally or marking this as "in progress".

Looks like this is working now though. I'm going to clean it up and squash it.

@Fishrock123
ci: run the linter from travis in a separate job
ee67fc0 
Divides the CI jobs by a custom `matrix:`, with a new linter job
using an existing node.js binary to run `make lint-ci` (eslint).

Refs: ayojs#71
@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Sep 21, 2017

Ok this should be good to merge if this passes, I think.

@TimothyGu
Copy link
Member

TimothyGu commented Sep 21, 2017

I'm okay with landing this without a macOS run. Travis is have some issues with their macOS infrastructure: https://www.traviscistatus.com/

@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Sep 21, 2017

Looks like it is building ok for us - its just taking a long tie to initially get started.

@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Sep 21, 2017

All green.

@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Sep 25, 2017

Can this be merged? 🤔

addaleax
addaleax approved these changes Sep 25, 2017
View reviewed changes
Copy link
Contributor

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

yeah, landing

addaleax pushed a commit that referenced this pull request Sep 25, 2017
@Fishrock123 @addaleax
ci: run the linter from travis in a separate job
8e2de77 
Divides the CI jobs by a custom `matrix:`, with a new linter job
using an existing node.js binary to run `make lint-ci` (eslint).

Refs: #71
PR-URL: #75
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax
Copy link
Contributor

addaleax commented Sep 25, 2017

Landed in 8e2de77

@addaleax addaleax closed this Sep 25, 2017
@Fishrock123 Fishrock123 deleted the ayo-linter branch September 25, 2017 21:57
addaleax added a commit to addaleax/node that referenced this pull request May 31, 2018
@addaleax @Fishrock123
build: initial .travis.yml implementation
30521c8 
Refs: ayojs/ayo#14
Refs: ayojs/ayo#75
Co-authored-by: Jeremiah Senkpiel <[email protected]>
@addaleax addaleax mentioned this pull request May 31, 2018
Closed
2 tasks
addaleax added a commit to nodejs/node that referenced this pull request Jun 7, 2018
@addaleax
build: initial .travis.yml implementation
977d011 
Refs: ayojs/ayo#14
Refs: ayojs/ayo#75
Co-authored-by: Jeremiah Senkpiel <[email protected]>

PR-URL: #21059
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit to nodejs/node that referenced this pull request Jun 8, 2018
@addaleax @targos
build: initial .travis.yml implementation
e36558a 
Refs: ayojs/ayo#14
Refs: ayojs/ayo#75
Co-authored-by: Jeremiah Senkpiel <[email protected]>

PR-URL: #21059
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit to nodejs/node that referenced this pull request Jun 13, 2018
@addaleax @targos
build: initial .travis.yml implementation
79b3423 
Refs: ayojs/ayo#14
Refs: ayojs/ayo#75
Co-authored-by: Jeremiah Senkpiel <[email protected]>

PR-URL: #21059
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@ljharb ljharb ljharb left review comments

@addaleax addaleax addaleax approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Fishrock123 @sandfox @TimothyGu @addaleax @ljharb