Page MenuHomePhabricator

MobileFrontend pre-commit hooks don't work on Windows (or if your shell is not Bash-compatible)
Closed, ResolvedPublic

Description

MobileFrontend pre-commit hooks don't work on Windows. They run commands defined in package.json "scripts", which are executed using the default command line (probably Bash on your Linux system, and cmd.exe on Windows), using a lot of Bash-specific syntax features (including: ** globs, setting env variables in the same command, subcommands in {}, relying on shebangs to run other shell scripts).

If you want to have non-trivial commit hooks, please write their code as normal separate files, instead of sticking them in package.json. (You have one good example, dev-scripts/svg_check.sh.)

(It's okay to use .sh scripts and Unixy executables, all of that can be installed on Windows and probably is already installed by most users with Git, but you can't assume that everything goes through Bash if you don't call it explicitly.)

Workaround is to use git commit -n. This is sad because it forces me to switch from my editor (which I usually use to commit changes often as I work on things) to command line :(

Event Timeline

matmarex created this task.Oct 27 2018, 7:35 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 27 2018, 7:35 PM
matmarex renamed this task from MobileFrontend pre-commit hooks don't work on Windows to MobileFrontend pre-commit hooks don't work on Windows (or if your shell is not Bash-compatible).Oct 27 2018, 7:35 PM

The precommit is important and I'd recommend not ignoring it. If it can be made to run in Windows that would be the most ideal outcome here. The problem here is none of my team use Windows so I'd be leaning on you here for a happy outcome (at least to test any solution).

If you want to have crazy commit hooks (I really wish you didn't, it is very inconvenient, they take like a minute or more to run), please write their code as normal separate files,

Could you define which one you consider "crazy" ?
I suspect you are referring to

git diff --name-status --exit-code resources/dist || { npm run node-debug; false; }"

and maybe

npm run node-debug

If so, of course this can be moved into a bash script. I suspect that would look like this? :

"test:bundle": "npm -s run build && npm run check-built-assets",
"check-built-assets": "./dev-scripts/check-built-assets.sh",

I haven't used a Windows machine for about 10 years now, so I'm not 100% sure what's needed here and how other projects solve these problems, so guidance very much appreciated.

Jdlrobson triaged this task as Low priority.Oct 30 2018, 9:41 PM

If you want to have crazy commit hooks (I really wish you didn't, it is very inconvenient, they take like a minute or more to run), please write their code as normal separate files,

Could you define which one you consider "crazy" ?

Maybe I should rephrase. I wasn't really thinking about anything in particular that they do, but about how long they take :) The pre-commit hook takes 20+ seconds (up to the point where it fails). I think that's crazy long. I like to commit after every self-contained change I make to the code, before I test it (so that I can undo the change easily) and this is definitely not compatible with that workflow. I will just get used to using git commit -n for this repo.

If so, of course this can be moved into a bash script. I suspect that would look like this? :

"test:bundle": "npm -s run build && npm run check-built-assets",
"check-built-assets": "./dev-scripts/check-built-assets.sh",

I haven't used a Windows machine for about 10 years now, so I'm not 100% sure what's needed here and how other projects solve these problems, so guidance very much appreciated.

Along these lines. You'll have to explicitly execute the script using sh:

"check-built-assets": "sh ./dev-scripts/check-built-assets.sh",

If you want to experience similar pains on your machine, try using a shell other than Bash, e.g. https://fishshell.com/ (apparently it even has different syntax for && too).


For the specific use case of checking if generated files have been committed, maybe you can steal the solution from VE, where we made it a grunt task: https://github.com/wikimedia/mediawiki-extensions-VisualEditor/blob/d80c383ad201e7b863acf6a6293acc0f2e6b1887/Gruntfile.js#L196. Surely that is preferable to Bash syntax (ew).

matmarex updated the task description. (Show Details)Oct 31 2018, 12:52 AM

I wasn't really thinking about anything in particular that they do, but about how long they take :)

I am somewhat sympathetic to this. I've also been using commit --no-verify while developing, but I find the hook important to make sure I don't waste fellow reviewers time with unmergeable code. Note: if you empty the precommit command in package.json and use a global gitignore to ignore package.json that will turn it off and may help with your workflow.

The "check-built-assets" job seems like a no brainer. Feel free to submit a patch, otherwise I may have some room to look into this in a week or 2.

Longer term, we won't need to commit assets every patchset. We'll hoping to move towards an OOjs UI like release process when the library is more stable and webpack built. The core code will probably move out the repo too. In the mean time.. apologies for the turbulence! I promise better things are on the horizon!

Change 471332 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Simplify check bundle job

https://gerrit.wikimedia.org/r/471332

I think the above will help @matmarex ... let me know if that can be improved in any way..

Change 471332 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] testing a bundle is more user/windows friendly

https://gerrit.wikimedia.org/r/471332

@matmarex how does the new precommit hook fare? Does that solve the issues you were hitting or at least make them more manageable?

I ended up deleting the precommit hook some time ago and I don't know how to get it back… (I don't know where it came from in the first place). I'm assuming that just runs npm run precommit.

Running npm run precommit still fails, but it seems that remaining issues are easy to solve.

The following changes get everything running on my machine:

diff --git a/package.json b/package.json
index da8d8c834..9b8bd5684 100644
--- a/package.json
+++ b/package.json
@@ -3,13 +3,13 @@
   "scripts": {
     "coverage": "nyc npm -s run test:unit",
     "start": "webpack -w --mode=development",
-    "build": "NODE_ENV=production webpack --mode=production",
+    "build": "set NODE_ENV=production && webpack --mode=production",
     "precommit": "composer test && npm run test:bundle-cached && npm run test:unit && npm run test:all",
-    "test:unit": "qunit 'tests/node-qunit/**/*.test.js'",
+    "test:unit": "qunit tests/node-qunit/**/*.test.js",
     "lint": "grunt test && npm run lint:es5 && npm run lint:js",
     "lint:es5": "eslint --no-eslintrc --no-ignore resources",
     "lint:js": "eslint --cache --max-warnings 0 --report-unused-disable-directives .",
-    "test:all": "npm run lint && npm run doc && dev-scripts/svg_check.sh",
+    "test:all": "npm run lint && npm run doc && bash dev-scripts/svg_check.sh",
     "test": "npm run -s test:bundle && npm run -s test:all",
     "test:bundle": "bash ./dev-scripts/check_bundle.sh 'HEAD^'",
     "test:bundle-cached": "bash ./dev-scripts/check_bundle.sh --cached",

I am not submitting to Gerrit because I'm not sure if it works with Bash, and I'm no longer interesting in pursuing this. The precommit checks take ~80 seconds on my machine. There's no way I will be using them. I'd rather run ESLint manually and push everything else to be checked by Jenkins.

Jdlrobson closed this task as Resolved.Nov 16 2018, 8:55 PM
Jdlrobson claimed this task.

I am not submitting to Gerrit because I'm not sure if it works with Bash, and I'm no longer interesting in pursuing this.

Okay! Let's call this done then?

FWIW your diff looks fine with the exception of set NODE_ENV=production. I think that needs to be export NODE_ENV=production in Unix environments.

Change 510505 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/MobileFrontend@master] More tweaks to NPM scripts to make them compatible with Windows

https://gerrit.wikimedia.org/r/510505

Change 510505 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] More tweaks to NPM scripts to make them compatible with Windows

https://gerrit.wikimedia.org/r/510505