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

Windows Development Fixes #6586

Merged
merged 2 commits into from
Jun 26, 2023
Merged

Windows Development Fixes #6586

merged 2 commits into from
Jun 26, 2023

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Jun 22, 2023

There are two separate commits in this PR:

Windows Line Endings

This makes it possible to do a fresh clone of this repo in Windows, and be able to run the resetdeps command successfully and then have a clean git status. We now check for a clean git status in during CI in Windows which was previously excluded.

The changes required were to:

  • Delete our workspace symlinks from node_modules and add a step to resetdeps that will do a platform dependent symlink before the initial reification
  • Add line ending normalization to our .gitattributes file and commit the result. Git will store text with LF line endings automatically, except for some files like cmd scripts that need to be crlf and shell scripts that need to be lf.
  • Update workflows to call node scripts/resetdeps.js directly since a fresh clone of the repo wont have any symlinks in place, so any node . command will fail due to missing workspaces.

Windows Shim Tests

The second commit refactors the shim tests to ensure the cmd scripts and all shells are tested in CI. It adds a new CI job that uses 3rd party actions to install WSL and Cygwin so that those can be tested.

These new tests would fail without the previous commit since the wrong line endings would cause the scripts to error.

Ref: npm/statusboard#562

@lukekarrys lukekarrys marked this pull request as ready for review June 22, 2023 05:39
@lukekarrys lukekarrys requested a review from a team as a code owner June 22, 2023 05:39
@wraithgar
Copy link
Member

Won't this make PRs from windows systems revert all these .cmd files?

@lukekarrys lukekarrys force-pushed the lk/workspace-symlinks branch 22 times, most recently from 37e0683 to 35f307d Compare June 23, 2023 03:23
@lukekarrys lukekarrys force-pushed the lk/workspace-symlinks branch from 35f307d to 1be9c1d Compare June 23, 2023 04:50
@lukekarrys
Copy link
Contributor Author

Won't this make PRs from windows systems revert all these .cmd files?

No since the files are normalized in git's index with LF line endings when committed but converted to CRLF on checkout when necessary.

@lukekarrys lukekarrys force-pushed the lk/workspace-symlinks branch from 8e6ca73 to 74f3a9d Compare June 23, 2023 16:30
@lukekarrys lukekarrys changed the title chore: normalize line endings and symlinks Windows Development Fixes Jun 23, 2023
@lukekarrys lukekarrys force-pushed the lk/workspace-symlinks branch from 74f3a9d to be47d43 Compare June 23, 2023 16:33
@lukekarrys lukekarrys marked this pull request as draft June 23, 2023 16:42
This makes it possible to do a fresh clone of this repo in Windows, and
be able to run the resetdeps command successfully and then have a clean
git status. We now check for a clean git status in during CI in Windows
which was previously excluded.

The changes required were to:
- Delete our workspace symlinks from node_modules and add a step to
  resetdeps that will do a platform dependent symlink before the initial
  reification
- Add line ending normalization to our `.gitattributes` file and commit
  the result. Git will store text with LF line endings automatically,
  except for some files like `cmd` scripts that need to be `crlf` and
  shell scripts that need to be `lf`.
- Update workflows to call `node scripts/resetdeps.js` directly since a
  fresh clone of the repo wont have any symlinks in place, so any
  `node .` command will fail due to missing workspaces.

Ref: npm/statusboard#562
@lukekarrys lukekarrys force-pushed the lk/workspace-symlinks branch 14 times, most recently from ea9e4a0 to 28f11aa Compare June 23, 2023 22:53
@lukekarrys lukekarrys force-pushed the lk/workspace-symlinks branch from 28f11aa to 6162f9f Compare June 23, 2023 22:54
@lukekarrys lukekarrys marked this pull request as ready for review June 23, 2023 23:05
@lukekarrys
Copy link
Contributor Author

In getting this tested I found that our bash scripts don't work under WSL v1, only WSL v2. I have WSL 2 installed locally and confirmed that the tests pass. GitHub Actions does not currently support WSL 2, so in that environment the test asserts that a new, more helpful error message is provided.

@wraithgar wraithgar merged commit 332dec3 into latest Jun 26, 2023
@wraithgar wraithgar deleted the lk/workspace-symlinks branch June 26, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants