| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Sorry, something went wrong.
|
@nodejs/platform-windows |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
cc @nodejs/testing |
Sorry, something went wrong.
|
Is cat included by default on windows machines? I know you get it through Git Bash, which we require, but I didn't think it came with CMD by default. |
Sorry, something went wrong.
|
@gibfahn I don't think cat is included. I just thought the build requirements was a safe condition for it. |
Sorry, something went wrong.
|
@gibfahn no, not by default. We already require Git Bash, tests already fail if the Unix tools are not installed, so I think it's better to make it consistent. @vsemozhetbyt you have the same issue in Refs: and Fixes:, you can leave only the Fixes: line. |
Sorry, something went wrong.
|
@vsemozhetbyt Yep, that was the context I hadn't seen. I think it's fine to assume the developer will have cat.exe on the system due to it being included in Git Bash, the only issue is that the developer might not have the path to the Git command tools in their CMD path. I guess the answer is to document that you will need to add C:\Program Files\Git\usr\bin to your system path if it's not there already. |
Sorry, something went wrong.
|
@gibfahn It seems something like that is already present in the doc:
|
Sorry, something went wrong.
That's the same with other tests too. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM. Happy to see stuff being removed from the common.js monolith!
Sorry, something went wrong.
|
This is causing failures for OSX testing on v6.x. did not cherry-pick. LMK if it should land |
Sorry, something went wrong.
post-mortem
bottom-lineBut in case changes like this are added I would like to ask for a quick failing sanity check to be added to run early to remind us if something in the environment is missing |
Sorry, something went wrong.
|
Such a test will also be a defacto documentation of what the following process is dependent on. |
Sorry, something went wrong.
Out of interest, what side effects are those?
A test that just runs through all the tools test.py expects makes sense to me, and not just for Windows. |
Sorry, something went wrong.
|
The post-morgen tag is for post mortem analysis related issues like core dump problems. Also, not sure why something not landing cleanly on an old version requires post-mortem analysis? |
Sorry, something went wrong.
Sorry. it's not documented (or I couldn't find where).
Since this has already landed, I could only consider this a post-mortem for it creating a regression on Windows. After my #12493 I didn't want to tag in "regression" since it's not such a big deal. |
Sorry, something went wrong.
For some reason if sh.exe is in the path, make can't run properly. Also it overrides find and if you don't order your %Path% correctly, it overrides some ported tools, like curl or wget. |
Sorry, something went wrong.
|
@gibfahn Ref: the Git4Windows installer https://github.com/git-for-windows/build-extra/blob/master/installer/install.iss#L863 |
Sorry, something went wrong.
|
FWIW the contributing guide mentions that the building guide should be read when running tests and the building guide does mention in the Windows section that basic unix tools are required for some tests and that they can be installed via Git for Windows. |
Sorry, something went wrong.
|
@mscdex aware, agree, but supporting docs with automation make for a time saver :-D |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Checklist
Affected core subsystem(s)
test
Fixes: #11469