Skip to content

fix: make build script work on Windows - #170

Open
hrithik577 wants to merge 3 commits into
jenkins-infra:mainfrom
hrithik577:fix/windows-build-script-169
Open

hrithik577 wants to merge 3 commits into
jenkins-infra:mainfrom
hrithik577:fix/windows-build-script-169

Conversation

@hrithik577

@hrithik577 hrithik577 commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Replace the shell-dependent cp command in the build script with a small Node.js script using node:fs/promises and copyFile.
  • Make generation of dist/404.html work cross-platform in Windows shells while retaining existing build behavior.

Fixes #169

Validation

  • Production build succeeds on Windows (npm run build).
  • Both dist/index.html and dist/404.html are generated and verified identical in content.
  • Lint checks pass with zero errors/warnings (npm run lint).
  • Prettier formatting check passes on modified files (npx prettier --check scripts/copy-404.js package.json).
  • Full test suite passes (npm test: 33 test files, 325 tests passed).

@Oaklaan

Oaklaan commented Oct 26, 2026

Copy link
Copy Markdown

Thanks for the fix, @hrithik577. Replacing the shell cp with a Node script
is a clean, cross-platform solution and matches what #169 asked for.

Looks good to me. One small suggestion: please make sure the script resolves
paths relative to the project root (e.g. via import.meta.url / path.resolve)
rather than the current working directory, and exits non-zero with a clear
message if dist/index.html is missing, so a broken build fails loudly.

Also worth confirming CI passes on Linux as well, since the Windows run is
the only one mentioned in the description.

@hrithik577

Copy link
Copy Markdown
Contributor Author

Hi @Oaklaan,

Thanks for the suggestions! I have updated scripts/copy-404.js:

Project-root relative paths: Paths are now resolved relative to import.meta.url using path.resolve(__dirname, '..', 'dist') (matching scripts/fetch-metadata.js), making execution independent of the working directory.
Explicit failure on missing source: Added await access(indexHtml) to verify dist/index.html exists before copying; if missing or if the copy fails, it prints a clear [ERROR] message and exits with status code 1.
Linux CI verification: The updated commit has run against the GitHub Actions CI workflow (Linux runner) and passed successfully.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues remain, and the shell-dependent copy step has been replaced with a cross-platform Node.js utility that includes validation and error handling.

No open issues 🎉

What changed in this PR

This pull request makes the production build cross-platform by replacing the shell-dependent cp command with a Node.js script that copies Vite’s generated index.html to dist/404.html.

Changes:

  • Added scripts/copy-404.js using platform-independent Node.js filesystem APIs.
  • Updated the production build script to invoke the new copy utility.
  • Added validation and error handling for missing files or failed copies.
File Description
scripts/​copy-404.js Validates and copies dist/index.html to dist/404.html using Node.js APIs.
package.json Replaces the shell-specific cp command in the production build pipeline.

Review effort: Lite

@PratikMane0112 PratikMane0112 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hrithik577 WDYT on these points ?

Comment thread scripts/copy-404.js Resolved
Comment thread scripts/copy-404.js Resolved
Comment thread scripts/copy-404.js Resolved
@Oaklaan

Oaklaan commented Oct 29, 2026

Copy link
Copy Markdown

@PratikMane0112 and @hrithik577. I’d like to help with some follow-ups that build on #170 and don’t overlap with the review comments above. I’m happy to adjust the scope, since I don’t know what’s already covered:

Windows CI check. If there isn’t one already, add a windows-latest job running npm ci && npm run build, so this kind of regression gets caught automatically

Test for the copy logic. A test in the repo’s existing test setup covering the success case and the missing index.html case

Docs. A short note in the README or CONTRIBUTING file about building on Windows.

@hrithik577

Copy link
Copy Markdown
Contributor Author

Hi @PratikMane0112,

Thanks again for your suggestions!

I've addressed all three points in the latest commit (b112dbe):

  • Combined the two try-catch blocks into one.
  • Updated the missing-file error to use a relative path.
  • Added a success confirmation message after copying 404.html.

I've also verified the production build, lint, Prettier, and missing-file error handling.

The full test suite encountered two timeout failures in an integration test under parallel execution on Windows; that test file passed when run independently.

I've pushed the changes to the existing PR. Thanks for your review!

@PratikMane0112

Copy link
Copy Markdown
Collaborator

@PratikMane0112 and @hrithik577. I’d like to help with some follow-ups that build on #170 and don’t overlap with the review comments above. I’m happy to adjust the scope, since I don’t know what’s already covered:

Windows CI check. If there isn’t one already, add a windows-latest job running npm ci && npm run build, so this kind of regression gets caught automatically

Test for the copy logic. A test in the repo’s existing test setup covering the success case and the missing index.html case

Docs. A short note in the README or CONTRIBUTING file about building on Windows.

@Oaklaan you can create separate issues for this & feel free to contribute as well, if interested. Lets keep the scope of this pr upto adding a node script.

@PratikMane0112
PratikMane0112 self-requested a review October 30, 2026 07:04

@PratikMane0112 PratikMane0112 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hrithik577 Thanks for pointing out issue & providing a fix. LGTM

@hrithik577

Copy link
Copy Markdown
Contributor Author

Hi @PratikMane0112,

Thanks for reviewing and approving PR #170. Just checking whether anything else is needed from my side before it can be merged.

All checks are passing, and I'm happy to make any further changes if required.

Thanks!

This branch has not been deployed

No deployments
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.

Make the build script work in Windows shells

4 participants