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

[WIP]tools: remove closure_linter to eslint on windows #1685

Closed
wants to merge 1 commit into from
Closed

[WIP]tools: remove closure_linter to eslint on windows #1685

wants to merge 1 commit into from

Conversation

yosuke-furukawa
Copy link
Member

This is a work in progress.
I found an error on Windows CI. gjslint still exists on vbuild.bat.
BUT, I don't have a Windows environment... So I have not confirmed this yet.
If I got our Jenkins permission, I will run the tests on Windows.

@mscdex mscdex added the build Issues and PRs related to build files or the CI. label May 12, 2015
@Fishrock123
Copy link
Contributor

R=@iojs/build?

@rvagg
Copy link
Member

rvagg commented May 12, 2015

when does closure_linter get removed from the tree? is this the last use of it?

also, @yosuke-furukawa has jenkins creds now, as do most other collaborators, it's probably just the newest collaborators that aren't set up now, they're welcome to ping me directly if they're not getting creds quick enough

@silverwind
Copy link
Contributor

I don't think Windows does * expansion in the shell, so instead you need a command line like this:

"%config%\iojs" tools\eslint\bin\eslint.js src lib --reset --quiet

Also, I couldn't get vcbuild jslint to run properly on my windows machine (it just builds and terminates), but that could be an issue on my side.

@silverwind
Copy link
Contributor

Oh, and let's not forget about the internal dir:

"%config%\iojs" tools\eslint\bin\eslint.js src lib lib\internal --reset --quiet

@silverwind
Copy link
Contributor

Nevermind, it seems specifying a directory makes eslint recursive automatically. so src lib is sufficient. I'll open a PR to fix that in the Makefile too.

@brendanashworth brendanashworth added the wip Issues and PRs that are still a work in progress. label May 12, 2015
@jbergstroem
Copy link
Member

I don't think you have to quote the command. LGTM after reverting to just specifying directories -- tested on one of the buildslaves (2012).

@silverwind
Copy link
Contributor

@jbergstroem did you test vcbuild jslint or the command directly?

@jbergstroem
Copy link
Member

@silverwind I replaced the old lines in vcbuild.bat with above and ran vcbuild.bat x64 nosign jslint.

@silverwind
Copy link
Contributor

Alright, LGTM then.

@yosuke-furukawa
Copy link
Member Author

jslint does not print error like python: can't open file 'tools/closure_linter/closure_linter/gjslint.py': [Errno 2] No such file or directory.
https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/674/nodes=win2012r2/console

I will land this.

yosuke-furukawa pushed a commit that referenced this pull request May 13, 2015
PR-URL: #1685
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Roman Reiss <me@silverwind.io>
@yosuke-furukawa
Copy link
Member Author

Landed. 966acb9

Thank you for review.

@yosuke-furukawa
Copy link
Member Author

@rvagg

when does closure_linter get removed from the tree? is this the last use of it?

Sorry for late response. We have discussed here. #1539

@silverwind
Copy link
Contributor

Minor nit I didn't notice earlier (sorry): path separator should be \ on Windows. It still works with / because of path normalization, but if we want to be fully correct, I'd fix that :)

@rlidwka
Copy link
Contributor

rlidwka commented May 17, 2015

It still works with / because of path normalization, but if we want to be fully correct, I'd fix that :)

Is there any practical reason to use backslashes? I mean if io.js/windows supports forward slashes, it might make sense to use them everywhere for consistency reasons.

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
PR-URL: nodejs#1685
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Roman Reiss <me@silverwind.io>
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. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants