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

fix: windows build with ping.js #836

Merged
merged 5 commits into from
Mar 24, 2023

Conversation

MikeMcC399
Copy link
Collaborator

@MikeMcC399 MikeMcC399 commented Mar 17, 2023

This PR fixes an issue building and committing the action on Microsoft Windows.

Issue

On Windows, compiling the source code using vercel/ncc would cause Windows-style CRLF end-of-line characters to be embedded in dist/index.js, but only for the portion of code coming from src/ping.js. Other parts of the file dist/index.js were using unix-style LF as end-of-line character.

Carrying out the same action on a unix system such as Ubuntu results in only LF characters being used.

This would lead to unnecessary git differences if the action were changed and committed to GitHub from unix and Windows in turn.

Resolution

The run format command is changed from:

"format": "prettier --write index.js" to
"format": "prettier --write index.js src/ping.js"

prettier is now used additionally on src/ping.js to pre-process this source file and convert the end-of-line character consistently to LF before it is fed to ncc build for compilation.

The Prettier: End of Line option is set to lf by default in v2.0.0 and is not overridden in .prettierrc.json. The action currently uses v2.8.4 of Prettier, so this default applies already.

This PR is a prerequisite for re-enabling a husky pre-commit hook (see also #743) so that it can work consistently across operating systems (unix / Windows).

Verification

Ubuntu

On Ubuntu 22.04, execute:

npm ci
npm run format
npm run build
git status

confirm the message:

"nothing to commit, working tree clean"

Microsoft Windows

Move to Microsoft Windows (11 Pro) and execute

git config --global core.autocrlf

Confirm that the default value of true is set (see Git Configuration). If not, then execute

git config --global core.autocrlf true

Now execute:

npm ci
npm run format
npm run build
git add .
git status

confirm the message:

"nothing to commit, working tree clean"

@jaffrepaul
Copy link
Contributor

There are two issues/PRs regarding ping.js updates. Can either of these be folded into this work to creating a larger update?

@MikeMcC399
Copy link
Collaborator Author

MikeMcC399 commented Mar 17, 2023

@jaffrepaul

There are two issues/PRs regarding ping.js updates. Can either of these be folded into this work to creating a larger update?

Sorry for the misunderstanding. I added clarification to the original post:

The run format command is changed from:

"format": "prettier --write index.js" to
"format": "prettier --write index.js src/ping.js"

All that this PR does is to make sure that CRLF is stripped off before src/ping.js is handed to ncc build for compilation in a Windows environment. On Ubuntu it's not an issue anyway.

This PR is not modifying the function of src/ping.js at all, so it does not make sense to combine the above PR and issue into this PR.

@MikeMcC399
Copy link
Collaborator Author

@AtofStryker
Will you arrange for a second reviewer for this PR? It is one of the prerequisites for supporting Renovate to become more automated when dependencies are involved that flow into the action build.

@AtofStryker
Copy link
Contributor

@MikeMcC399 I can take care of that. @lmiller1990 can you take a look here as well?

@lmiller1990
Copy link
Contributor

🚀

@lmiller1990 lmiller1990 merged commit 34dad9d into cypress-io:master Mar 24, 2023
@MikeMcC399 MikeMcC399 deleted the fix/action-build branch March 24, 2023 05:24
@AtofStryker AtofStryker removed their assignment Mar 28, 2023
@github-actions
Copy link

🎉 This PR is included in version 5.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants