fix: windows build #20854

estrada9166 · 2022-03-30T22:22:14Z

Closes UNIFY-1377

User facing changelog

Additional details

Currently, the build on windows machines is not working as expected because there's a bash script that is not supported unless extra packages are installed on the machine.

Also, rm is not supported at all, so there's the need to update the usage of it for rimraf which is fully supported!

Update failing migration test where the file path is not matching on windows machines

How has the user experience changed?

PR Tasks

Have tests been added/updated?
Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?
Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2022-03-30T22:22:17Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-03-31T15:42:39Z

Test summary

17792 • 0 • 217 • 0 • 2

Run details


Project	cypress
Status	Passed
Commit	`95dfb13`
Started	Apr 4, 2022 3:29 PM
Ended	Apr 4, 2022 3:41 PM
Duration	11:49 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	cypress/e2e/commands/xhr.cy.js		2
1	... > no status when request isnt forced 404
2	src/cy/commands/xhr > abort > aborts xhrs currently in flight

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

elevatebart · 2022-03-31T15:55:34Z

I believe bash was already required for run-if-not-ci.sh

elevatebart · 2022-03-31T16:02:53Z

I like TypeScript as it is easier to debug than bash.

ZachJW34 · 2022-03-31T16:34:24Z

packages/icons/scripts/build.ts

+
+  await exec('yarn tsc -p ./tsconfig.build.json')
+
+  if (os.platform() === 'darwin') {


Not related to this PR, but does this platform specific build have any effect on what the binary expects? When we build the Cypress binary for windows, will it have dist/icons/cypress.icns available and is it needed? Maybe @elevatebart has an idea

@ZachJW34, the answer is no and it does not need it. the only moment where we "need" the icns file is when we package the electron app for mac. icns is the mac specific icon format that contains multiple definitions. Since this electron app can only be built and signed on Mac, I am not afraid of this happening.

tbiethman · 2022-03-31T23:34:51Z

package.json

@@ -40,7 +40,7 @@
    "effective:circle:config": "circleci config process circle.yml | sed /^#/d",


The 'clean-deps' command here at root is still using rm

cypress/package.json

Line 21 in 9f921a0

"clean-deps": "find . -depth -name node_modules -type d -exec rm -rf {} \\;",

Yeah! I didn't changed bc it was changed 2 years ago and seems not to be an issue - but may worth it changing to be consistent across all the packages

tbiethman · 2022-03-31T23:42:39Z

packages/config/package.json

-    "clean-deps": "rm -rf node_modules",
-    "clean": "rm -f ./src/*.js ./src/**/*.js ./src/**/**/*.js ./test/**/*.js || echo 'cleaned'",
+    "clean-deps": "rimraf node_modules",
+    "clean": "rimraf ./src/*.js ./src/**/*.js ./src/**/**/*.js ./test/**/*.js || echo 'cleaned'",


These clean commands that delete implementation files are wild; are they actively used?

It seems those are actively used - the screenshot is running yarn in 10.0-release branch

Yeah, we don't use dist folders at cypress to avoid navigating to type files when changing packages. source maps would be a solution, but it does not seem to be one we considered for dev. we prefer using ts-node.

When we run build-prod, on local, it will create a bunch of files that need to be cleaned up.

…io/cypress into alejandro/fix/fix-windows-build

lmiller1990

Code seems fine but how did you test this on windows? Are you using windows? Should we enable windows for this PR using circle.yml to see if it actually all works?

elevatebart · 2022-04-04T02:40:18Z

I tested it on windows.

lmiller1990 · 2022-04-04T09:26:16Z

Ok - so once this merges, and windows runs on the 10.0-release it should be green? Are there any outstanding windows problems?

I'll leave merging this to @estrada9166 🙏

marktnoonan · 2022-04-04T11:05:09Z

I'm going to make a branch off of this now that runs the windows tests, so we can see them run in CI before merging to 10.0-release without modifying this branch's circle.yml, I will update with results when they have completed.

marktnoonan

Looks like we need a little more logic in scripts/run-postInstall.js - or just to remove the CI check, I haven't looked in detail yet, just wanted to make sure we don't merge yet. I'll look into it a bit more and try some changes on the win-fix-windows-build-ci branch. If that starts passing I'll make a PR into this with the changes.

marktnoonan · 2022-04-04T11:40:07Z

scripts/run-postInstall.js

+const os = require('os')
+const { execSync } = require('child_process')
+
+if (os.platform() === 'win32' && !process.env.CI) {


Excluding CI for this caused the postinstall step in the windows-node-modules-install job to fail on Windows in CI due to the non-windows version of this code running. Can we remove the CI check safely, or do we need another condition for windows on CI?

Here is a copy run based on a copy of this branch with windows tests enabled in CI. We see this error:

Error: Command failed: patch-package && ./scripts/run-if-not-ci.sh yarn-deduplicate --strategy=highest && yarn clean && gulp postinstall && ./scripts/run-if-not-ci.sh yarn build

Confirmed this can be fixed with an extra conditional, but with that in mind the whole file can be simplified, since we are only splitting things up due to the pattern of using ./scripts/run-if-not-ci.sh.

I'd like to propose removing run-if-not-ci.sh completely and just having a CI version and local version of what we want, that works on all platforms:

const { execSync } = require('child_process') const executionEnv = process.env.CI ? 'ci' : 'local' const postInstallCommands = { local: 'patch-package && yarn-deduplicate --strategy=highest && yarn clean && gulp postinstall && yarn build', ci: 'patch-package && yarn clean && gulp postinstall', } execSync(postInstallCommands[executionEnv], { stdio: 'inherit', })

marktnoonan

Approving this now as the windows build succeeds, I expect a few windows tests may continue to fail in CI after this PR based on letting CI run already in a different branch, but that's fine, this PR is not intended to fix all outstanding windows test issues.

* fix: update icons build to work on windows * Add rimraf * Update scripts * Update script * Update scripts * Fix windows migration * Update postinstall script * Remove script (cherry picked from commit e6cbc5a)

* 10.0-release: (92 commits) chore: remove dependency-tree dep (#20905) chore(launchpad): work on infra for scaffold tests (#20818) fix: build mjs in the cli (#20889) fix(unify): Cypress version link should point to Cypress doc's changelog (#20869) fix: windows build (#20854) fix: add index.mjs to the published files of cli (#20884) refactor: lift indexHtmlFile up to component, add validation (#20870) fix: allow migration of pluginsFile using `env` properties (#20770) fix: viewport from CLI on CT (#20849) Don't communicate if process isn't connected Remove config.get Update packages/data-context/src/data/ProjectLifecycleManager.ts fix: git data source unit test failure (#20875) add comment for autoBindDebug fix: Ensuring current browser is synchronized between app and launchpad (#20830) Fix missed await on merge conflict resolution test(unification): move record keys to contexts (#20860) test: move record keys to contexts (#20859) PR comments Fix tests that visit app without a configured testing type ...

fix: update icons build to work on windows

eb69581

Merge branch '10.0-release' into alejandro/fix/fix-windows-build

a958326

estrada9166 marked this pull request as ready for review March 31, 2022 15:24

estrada9166 requested review from elevatebart and BlueWinds March 31, 2022 15:25

elevatebart previously approved these changes Mar 31, 2022

View reviewed changes

Merge branch '10.0-release' into alejandro/fix/fix-windows-build

04432ab

estrada9166 marked this pull request as draft March 31, 2022 16:15

estrada9166 changed the title ~~fix: update icons build to work on windows~~ fix: windows build Mar 31, 2022

Add rimraf

57ee185

estrada9166 dismissed elevatebart’s stale review via 57ee185 March 31, 2022 16:39

ZachJW34 reviewed Mar 31, 2022

View reviewed changes

estrada9166 and others added 3 commits March 31, 2022 16:35

Update scripts

7787339

Update script

9586945

Merge branch '10.0-release' into alejandro/fix/fix-windows-build

9f921a0

estrada9166 marked this pull request as ready for review March 31, 2022 22:11

estrada9166 requested review from a team as code owners March 31, 2022 22:11

elevatebart previously approved these changes Mar 31, 2022

View reviewed changes

tbiethman reviewed Mar 31, 2022

View reviewed changes

estrada9166 added 2 commits March 31, 2022 18:59

Update scripts

5ecca21

Merge branch 'alejandro/fix/fix-windows-build' of github.com:cypress-…

77b4434

…io/cypress into alejandro/fix/fix-windows-build

estrada9166 dismissed elevatebart’s stale review via 77b4434 April 1, 2022 00:00

estrada9166 requested a review from tbiethman April 1, 2022 00:03

Merge branch '10.0-release' into alejandro/fix/fix-windows-build

5fcc2c6

elevatebart previously approved these changes Apr 1, 2022

View reviewed changes

Fix windows migration

17ff038

estrada9166 dismissed elevatebart’s stale review via 17ff038 April 1, 2022 15:39

Merge branch '10.0-release' into alejandro/fix/fix-windows-build

b76df0d

elevatebart previously approved these changes Apr 1, 2022

View reviewed changes

Merge branch '10.0-release' into alejandro/fix/fix-windows-build

f866717

lmiller1990 self-requested a review April 3, 2022 20:57

lmiller1990 previously approved these changes Apr 3, 2022

View reviewed changes

marktnoonan requested changes Apr 4, 2022

View reviewed changes

Update postinstall script

9e121d8

estrada9166 dismissed stale reviews from lmiller1990 and elevatebart via 9e121d8 April 4, 2022 14:58

tbiethman previously approved these changes Apr 4, 2022

View reviewed changes

estrada9166 requested a review from marktnoonan April 4, 2022 15:16

Remove script

95dfb13

estrada9166 dismissed tbiethman’s stale review via 95dfb13 April 4, 2022 15:22

marktnoonan approved these changes Apr 4, 2022

View reviewed changes

estrada9166 requested review from tbiethman, elevatebart, lmiller1990 and ZachJW34 April 4, 2022 15:36

tbiethman approved these changes Apr 4, 2022

View reviewed changes

estrada9166 merged commit e6cbc5a into 10.0-release Apr 4, 2022

estrada9166 deleted the alejandro/fix/fix-windows-build branch April 4, 2022 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: windows build #20854

fix: windows build #20854

estrada9166 commented Mar 30, 2022 •

edited

cypress-bot bot commented Mar 30, 2022

cypress bot commented Mar 31, 2022 •

edited

elevatebart commented Mar 31, 2022

elevatebart commented Mar 31, 2022

ZachJW34 Mar 31, 2022

elevatebart Apr 1, 2022

tbiethman Mar 31, 2022 •

edited

estrada9166 Mar 31, 2022

tbiethman Mar 31, 2022

estrada9166 Apr 1, 2022

elevatebart Apr 1, 2022

elevatebart Apr 1, 2022

lmiller1990 left a comment

elevatebart commented Apr 4, 2022

lmiller1990 commented Apr 4, 2022

marktnoonan commented Apr 4, 2022

marktnoonan left a comment

marktnoonan Apr 4, 2022

marktnoonan Apr 4, 2022

marktnoonan left a comment


		await exec('yarn tsc -p ./tsconfig.build.json')

		if (os.platform() === 'darwin') {

		@@ -40,7 +40,7 @@
		"effective:circle:config": "circleci config process circle.yml \| sed /^#/d",

fix: windows build #20854

fix: windows build #20854

Conversation

estrada9166 commented Mar 30, 2022 • edited

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Mar 30, 2022

cypress bot commented Mar 31, 2022 • edited

Test summary

Run details

Flakiness

elevatebart commented Mar 31, 2022

elevatebart commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbiethman Mar 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

elevatebart commented Apr 4, 2022

lmiller1990 commented Apr 4, 2022

marktnoonan commented Apr 4, 2022

marktnoonan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marktnoonan left a comment

Choose a reason for hiding this comment

estrada9166 commented Mar 30, 2022 •

edited

cypress bot commented Mar 31, 2022 •

edited

tbiethman Mar 31, 2022 •

edited