Page MenuHomePhabricator

Make all Node-based linters properly use `shouldUseInterpreter`
AbandonedPublic

Authored by BYK on Aug 11 2014, 5:09 PM.
Tags
None
Referenced Files
F14112713: D10220.diff
Thu, Nov 28, 2:22 AM
Unknown Object (File)
Wed, Nov 20, 7:35 AM
Unknown Object (File)
Sat, Nov 16, 4:05 AM
Unknown Object (File)
Mon, Nov 11, 11:26 PM
Unknown Object (File)
Fri, Nov 8, 1:54 AM
Unknown Object (File)
Oct 2 2024, 1:11 AM
Unknown Object (File)
Sep 2 2024, 4:31 PM
Unknown Object (File)
Aug 27 2024, 12:42 PM
Subscribers

Details

Summary

Node-based linters did not define shouldUseInterpreter which defaulted
to false, rendering them useless on Windows because the strict config
check (interpreter field is not allowed unless shouldUseInterpreter
returns true).

This patch fixes the issue. We can probably have a NodeBasedLinter
base class to abstract away some commonalities in the future.

Test Plan

No idea.

Diff Detail

Repository
rARC Arcanist
Branch
fix-jshint
Lint
Lint Passed
Unit
Test Failures
Build Status
Buildable 2152
Build 2156: [Placeholder Plan] Wait for 30 Seconds

Unit TestsFailed

TimeTest
0 mstestJSHintLinter
0 mstestPuppetLintLinter
0 mstestFixLetterCase
0 mstestJSONLinter
0 mstestMergeConflictLint
View Full Test Results (2 Failed · 9 Passed · 14 Skipped)

Event Timeline

BYK retitled this revision from to Make all Node-based linters properly use `shouldUseInterpreter`.
BYK updated this object.
BYK edited the test plan for this revision. (Show Details)
BYK added a reviewer: epriestley.

This will take a long time for me to review, so I probably won't get to it for a while.

I need to convince myself this is the best approach before I accept this. To do that, I need to research the answers to a lot of questions that I'm not very familiar with (like "Why doesn't node install binaries on Windows?"), reproduce the issue, etc.

I need to convince myself this is the best approach before I accept this. To do that, I need to research the answers to a lot of questions that I'm not very familiar with (like "Why doesn't node install binaries on Windows?"), reproduce the issue, etc.

I mean, I just used the existing ArcanistExternalLinters facilities (the interpreter related parts) that said I understand your concerns. The reason is Windows not supporting shebangs, otherwise it has the "binaries" but you cannot run ./node_modules/jshint/bin/jshint directly for instance because it doesn't have a file extension, thus doesn't have an associated program to run it (node) and Windows does not support doing this automatically with the help of shebangs.

On some occasions you may have .cmd files which can be directly run from the command line by typing just the name (without the extension) but when you try to run it programmatically, you need to provide the full path, thus the .cmd part too which differs from Unix-like systems so I'd say the safest and most compatible approach is the current "separate interpreter/binary" approach.

You are of course free to do your own research and validate what I said above :)

Apparently this is not gonna happen so abandoning.