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

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

Closed
wants to merge 1 commit into from
Closed

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

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
@@ -18,3 +18,4 @@ install:
- make -j2 V=
script:
- make -j2 test-ci
- make lint
Copy link

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

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

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

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

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

Where is run-ci.sh used?

@Fishrock123
Copy link
Contributor Author

@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

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

@TimothyGu
Copy link
Member

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

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

@Fishrock123
Copy link
Contributor Author

All green.

@Fishrock123
Copy link
Contributor Author

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 <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax
Copy link
Contributor

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 <fishrock123@rocketmail.com>
@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 <fishrock123@rocketmail.com>

PR-URL: #21059
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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 <fishrock123@rocketmail.com>

PR-URL: #21059
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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 <fishrock123@rocketmail.com>

PR-URL: #21059
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ljharb ljharb ljharb left review comments

@TimothyGu TimothyGu TimothyGu approved these changes

@addaleax addaleax addaleax approved these changes

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