Set up CI on Windows #344

lishaduck · 2025-02-21T21:06:34Z

Resolves #343.

~~Should probably fail until #340 gets fixed?~~

jfmengels · 2025-02-21T21:53:38Z

I agree we should run tests on Windows (maybe on Mac as well?), but I imagine it will increase the likelyhood that we get rate limits (which we still keep seeking for some reason). Should we maybe exclude the template tests on Windows?

lishaduck · 2025-02-21T21:59:50Z

First off, how did I hit rate limits without running it? Must've still got something wrong with #310. I'll take a look ASAP.
Second off, it luckily looks like most of this so far is newlines. A few path issues in test code, but I didn't see any bugs (which are probably in the run tests).

socket-security · 2025-02-23T00:15:56Z

New and updated dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/[email protected]	None	`0`	51.3 kB	danielroe, pi0
npm/[email protected] 🔁 npm/[email protected]	Transitive: filesystem	`+2`	130 kB

View full report↗︎

lishaduck · 2025-02-23T00:25:52Z

test/run.mjs

-  return data.replace(new RegExp(localPath, 'g'), '<local-path>');
+  return data
+    .replace(
+      // eslint-disable-next-line security/detect-non-literal-regexp -- Test code.


I'm not happy about this either. Perhaps we should only enable security on non-test files?

Depends what other rules security holds. I would like it help prevent if people try to do weird code executions for instance. But yeah, a regex like this is fine.

lishaduck · 2025-02-23T00:29:29Z

Wow, pathe is magic! All tests failing to just one failing because Anonymize needs updates.

jfmengels · 2025-02-23T22:23:53Z

lib/watch.js

@@ -383,18 +376,15 @@ function createExtraFilesWatcher(options, app, runReview, onError, request) {
  return chokidar
    .watch(
      request.files.map(
-        ({pattern, included}) =>
-          (included ? '' : '!') + OsHelpers.makePathOsAgnostic(pattern)
+        ({pattern, included}) => (included ? '' : '!') + pattern


Does chokidar support Windows-style paths?

~~No idea. I should check.~~ They normalize it (at least they used to, we're still on v3, so they still should?), so you can use either, but it should return a POSIX path rather than a DOS path.
I had a chat with my dad about the elm-review's handling of drives, which basically ended with makePathOsAgnostic uses /.:/ to strip the drive, when elm-review should keep the drive so that you can execute it across drives (which MS wants to make more common). pathe handles proper agnosticism, so the pattern passed in should be correct, but I should also just check that as well.

elm-format doesn't look like it supports lf line endings on Windows. Also disable remote tests on Windows.

I think this is partly a step in the wrong direction, but we'll see. Here's what I've found about drive letters: - On normal Windows, paths are DOS paths, like "C:\path\to\file" - Except when they're not, like - "\\.\C:\path\to\file" - "\\?\C:\path\to\file" - Sometimes Windows uses UNC, which is like "\\hostname\C$\path\to\file" - On Git Bash, paths are like "/c/path/to/file" - On Node, paths are like "C:/path/to/file" - On WSL, paths are like "/mnt/c/path/to/file" Sometimes drive letters are case sensitive, sometimes they're not. And that's just for absolute paths. There's even more weird stuff for relative paths. Makes me glad to be using macOS so much these days.

Yes, this should be fixed. No, this should be fixed later. For now, I need to check the tests.

lishaduck · 2025-02-23T23:00:33Z

@jfmengels, how much do we care about outputting idiomic paths on Windows? Right now, the CLI outputs paths like this: "C:\Somewhere\Foo.txt", but this PR changes that to "C:/Somewhere/Foo.txt", which is equally valid, but could be a slight behavioral change (does that hurt the IDE integration?) and might be confusing to newcomers? (But Windows paths are confusing in general, so maybe they should just get over it, see the writeup at b9c822a).

lishaduck · 2025-02-24T00:32:15Z

@jfmengels, can you take a look a the current failure? I'm not really sure why it's different. Is it an prompts thing? I don't think we're doing anything different.

lishaduck · 2025-02-24T00:44:47Z

Is it an prompts thing?

Yup. This is silly code: https://github.com/terkelg/prompts/blob/e0519913ec4fcc6746bb3d97d8cd0960c3f3ffde/lib/util/figures.js

So we could normalize it, we could try inquirer or Clack or something, IDK, maybe we could monkey-patch the characters?
Can Windows really not render the characters by default?

lishaduck · 2025-02-24T01:09:33Z

Can Windows really not render the characters by default?

If I'm reading this old DevBlogs post correctly, UTF-8 support for Windows Console landed in 2018, so it should be fine by now.

jfmengels · 2025-02-24T16:08:39Z

maybe we could monkey-patch the characters?

Yeah, I guess we should... 😞
I would be okay with replacing prompts with a custom thing as well, but that's a bigger task 😅

jfmengels · 2025-02-24T16:19:47Z

how much do we care about outputting idiomic paths on Windows? Right now, the CLI outputs paths like this: "C:\Somewhere\Foo.txt", but this PR changes that to "C:/Somewhere/Foo.txt", which is equally valid, but could be a slight behavioral change (does that hurt the IDE integration?) and might be confusing to newcomers? (But Windows paths are confusing in general, so maybe they should just get over it, see the writeup at b9c822a).

That's a lot of ways to write Windows paths 🤔
I honestly don't know. If they're bot h valid, then either one is fine by me, but since I'm not a Windows dev, I'm not adequately positioned to to tell what is best. I would say that it's probably best to change this only it it is nicer for the user in some way, not if it's just for us running it in CI.

lishaduck · 2025-02-24T16:24:40Z

I would say that it's probably best to change this only it it is nicer for the user in some way, not if it's just for us running it in CI.

Ok, that was my question. I'll see if I can normalize back to Windows for output.

lishaduck · 2025-02-24T16:27:46Z

I would be okay with replacing prompts with a custom thing as well, but that's a bigger task 😅

I took a closer look at alternatives. Clack is a very different feel from elm-review's current vibe, I don't think we want to go that way. inquirer also detects windows, but it also supports unicode on modern terminals, so perhaps we could fake the terminal env var in tests to be one that supports unicode? Maybe???
I might need to look into the prior art. Somebody's got to have run into this before.

Honestly, I might just find-replace √ with ✔ for now.

lishaduck force-pushed the windows-ci branch 2 times, most recently from e37d941 to c3c75bf Compare February 22, 2025 23:10

lishaduck changed the title ~~Setup ci on windows~~ Set up CI on Windows Feb 22, 2025

lishaduck commented Feb 23, 2025

View reviewed changes

jfmengels reviewed Feb 23, 2025

View reviewed changes

lishaduck force-pushed the windows-ci branch from 3623ee3 to 39429b5 Compare February 23, 2025 22:46

lishaduck mentioned this pull request Feb 23, 2025

Report errors if no files were found or if source-directories are empty #352

Merged

lishaduck added 12 commits February 23, 2025 16:51

ci: setup ci on windows

28fffc0

ci: less windows

0ee229c

elm-format doesn't look like it supports lf line endings on Windows. Also disable remote tests on Windows.

ci: replace CRLF with LF on Windows

391d06e

fix: checkout with lf line endings

f17acf2

ci: better matrix

57b4beb

ci: try pathe

45ebf74

fix: shhhh lint

02f2d25

Yes, this should be fixed. No, this should be fixed later. For now, I need to check the tests.

ci: still run engines test

afc53a3

do we still need this?

91d7a9d

chore: add comment

00ede70

fix: don't use pathe in anonymize

62928c5

lishaduck force-pushed the windows-ci branch from 39429b5 to 62928c5 Compare February 23, 2025 22:52

lishaduck added 2 commits February 23, 2025 17:03

fix: anonymize more

3a962ed

fix: normalizer

a54de65

lishaduck force-pushed the windows-ci branch from ca83b24 to a54de65 Compare February 23, 2025 23:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up CI on Windows #344

Set up CI on Windows #344

lishaduck commented Feb 21, 2025 •

edited

Loading

jfmengels commented Feb 21, 2025

lishaduck commented Feb 21, 2025

socket-security bot commented Feb 23, 2025 •

edited

Loading

lishaduck Feb 23, 2025

jfmengels Feb 23, 2025

lishaduck commented Feb 23, 2025

jfmengels Feb 23, 2025

lishaduck Feb 23, 2025

lishaduck commented Feb 23, 2025

lishaduck commented Feb 24, 2025

lishaduck commented Feb 24, 2025 •

edited

Loading

lishaduck commented Feb 24, 2025

jfmengels commented Feb 24, 2025

jfmengels commented Feb 24, 2025

lishaduck commented Feb 24, 2025

lishaduck commented Feb 24, 2025 •

edited

Loading

Set up CI on Windows #344

Are you sure you want to change the base?

Set up CI on Windows #344

Conversation

lishaduck commented Feb 21, 2025 • edited Loading

jfmengels commented Feb 21, 2025

lishaduck commented Feb 21, 2025

socket-security bot commented Feb 23, 2025 • edited Loading

lishaduck Feb 23, 2025

Choose a reason for hiding this comment

jfmengels Feb 23, 2025

Choose a reason for hiding this comment

lishaduck commented Feb 23, 2025

jfmengels Feb 23, 2025

Choose a reason for hiding this comment

lishaduck Feb 23, 2025

Choose a reason for hiding this comment

lishaduck commented Feb 23, 2025

lishaduck commented Feb 24, 2025

lishaduck commented Feb 24, 2025 • edited Loading

lishaduck commented Feb 24, 2025

jfmengels commented Feb 24, 2025

jfmengels commented Feb 24, 2025

lishaduck commented Feb 24, 2025

lishaduck commented Feb 24, 2025 • edited Loading

lishaduck commented Feb 21, 2025 •

edited

Loading

socket-security bot commented Feb 23, 2025 •

edited

Loading

lishaduck commented Feb 24, 2025 •

edited

Loading

lishaduck commented Feb 24, 2025 •

edited

Loading