Fix build, lint, and (most) test targets on Windows
#2717
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… 'dom' types not being present, I'm not sure why this works on other platforms
…) working differently, we have to loosen one lint rule that won't work due to to way typescript-eslint walks symbolic links in a non-agnostic way. building and resolution still seems to work properly.
…m shell support, and 2) fixes some critical holes that can lead to remote code execution. nice side effect, the new types pointed out a typo (utf-8 instead of utf8 for a param).
… explicit help to propertly root the test discovery
…mock getEnvironmentInfo(), which is necessary on Windows and makes the test run LOT faster.
…perating systems and filesystems (eg on linux/bsd).
…-fix the build. powershell emits an extra CR after process terminates, so trim() the stderr output to make the snapshot fulfill its intent in an OS-agnostic way
matthargett
requested review from
cipolleschi,
cortinico,
szymonrybczak and
thymikee
as code owners
…stem combinations. it looks like there's a latent race in e2e tests where sometimes a directory doesn't exist yet, or was already removed. this isn't a new issue, so check for directory before removal. trim stdout/stderr in the test helpers, not just in one test.
|
There hasn't been any activity on this pull request in the past 3 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Right now, a fresh clone of this repository does not work on Windows under either PowerShell or cmd prompts. This is due to some simple reasons (OS-specific path concatenation), and some tricky ones (Windows filesystems' "junction points" are incompatible with how current eslint walks symlinks). This pull request addresses critical issues that prevented the project from being built and tested on Windows environments. The primary goal was to ensure cross-platform compatibility for the build scripts and resolve related test failures, thereby unblocking contributions from developers on Windows platforms.
Details
The core of this PR is fixing the build process on Windows. Previously, running
yarn buildwould fail due to a combination of non-portable code in build scripts and issues with how the TypeScript composite build handled incremental builds on Windows. Additionally, several snapshot tests were failing due to differences in filesystem ordering between inode-derived file systems and Windows.This PR introduces the following fixes:
build-clean-allscript now properly cleans out stale TypeScript build information (.tsbuildinfofiles) along with build artifacts. This resolves persistent issues with the composite build failing incorrectly even when making fixes to the build script/infra.execafor Security and Compatibility: Theexecapackage was upgraded to the latest version. This not only improves cross-platform command execution, particularly on Windows, but highlighted a latent typo (utf-8versusutf8), and also resolves latent security vulnerabilities related to command injection.A Note on Remaining Test Failures
While this PR fixes the build and a specific class of test failures, a number of unrelated unit and end-to-end tests are still failing. These failures appear to be pre-existing and are not further regressions on Windows caused by these changes.
On Windows, we disable a lint error that relates to workspace resolution. These failures appear to be an issue with how eslint processes project files when they are symbolic links (junction points on Windows file system). This led to incorrect file resolution during linting. This appears to be a known issue within the ecosystem, with developers reporting similar behavior in other contexts.
To keep this pull request focused on the critical task of unblocking the build, I'm deferrinf the fix for the few remaining test failures in a separate, follow-up effort. This allows us to merge the foundational build fixes quickly and enables developers on all platforms to contribute to fixing the remaining issues. Until all relevant tests are passing and Windows CI is introduced, I don't know that we should advertise Windows support in the docs. I'm open to feedback on this aspect.
Test Plan
On Windows:
yarn lintyarn buildyarn test(still some failures, but starting point was zero tests passing)Checklist
react-nativecheckout (instructions).