Fix Windows installation issue by correcting null device path in install.bat script#5910
Conversation
WalkthroughThe updates include a modification to a Windows batch script to use the correct null device for output redirection and an adjustment to the Vite configuration to mark several Changes
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.gitignore (1)
35-35: Exclude generated component artifacts.
Ignoring/packages/app/src/components/__generated__/*keeps auto-generated files out of version control.
Consider simplifying to ignore the directory and all nested content, for example:/packages/app/src/components/__generated__/or using a glob like:
**/__generated__/**
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)install.bat(1 hunks)
🔇 Additional comments (2)
.gitignore (1)
33-33: Ignore.envto protect sensitive configurations.
Adding.envto.gitignoreprevents local environment variables (which often contain secrets or API keys) from being inadvertently committed.install.bat (1)
44-44: Use Windowsnulinstead of/dev/null.
Replacing the Unix-style/dev/nullwith Windowsnulcorrectly suppressesyarn installoutput in CMD, fixing the “path not found” error during installation.
|
I don't see a code owner listed for install.bat. @benjaminpkane I see you authored the most recent PR for this file and @CamronStaley I see you reviewed it #5769. Would you be able to take a look at this? |
benjaminpkane
left a comment
There was a problem hiding this comment.
Hi @chengolivia. Thank you for the fix! We actually would like __generated__ files as a part of source control for convenience and tracking purposes. Can you remove that .gitignore change?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/packages/app/vite.config.ts (1)
32-38: Good choice marking packages as external dependencies.Marking these
@fiftyonepackages as external prevents them from being bundled, which is appropriate for packages that should be loaded at runtime rather than bundled. This change complements the installation process improvements mentioned in the PR objectives.However, the array formatting could be improved for better readability.
external: [ - '@fiftyone/analytics', '@fiftyone/components', '@fiftyone/core', - '@fiftyone/relay', '@fiftyone/state', '@fiftyone/utilities', - '@fiftyone/looker', '@fiftyone/map', '@fiftyone/plugins', - '@fiftyone/embeddings', '@fiftyone/operators', '@fiftyone/fiftyone', - '@fiftyone/aggregations', '@fiftyone/flashlight' + '@fiftyone/analytics', + '@fiftyone/components', + '@fiftyone/core', + '@fiftyone/relay', + '@fiftyone/state', + '@fiftyone/utilities', + '@fiftyone/looker', + '@fiftyone/map', + '@fiftyone/plugins', + '@fiftyone/embeddings', + '@fiftyone/operators', + '@fiftyone/fiftyone', + '@fiftyone/aggregations', + '@fiftyone/flashlight' ],
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/packages/app/vite.config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/app/vite.config.ts
Also, I added an update to |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5910 +/- ##
========================================
Coverage 99.10% 99.10%
========================================
Files 76 76
Lines 21036 21117 +81
========================================
+ Hits 20848 20929 +81
Misses 188 188
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
A lint check is failing. Otherwise LGTM! |
|
Still an e2e issue. Keeping this just to the bat script change is best |
Head branch was pushed to by a user without write access
Ok, I reverted the other changes. Now the PR just handles the bat script change. I had added the other commits because I was getting rollup errors running install.bat. But a rm of node_modules and a fresh install.bat did the trick. |
|
Here is the error I was getting before I removed and re-installed node_modules: Removing node_modules and re-running install.bat fixed the problem, so now the app is building without those other commits. However, the fresh build outputted the following warning which could be related to the error I got before: |
|
@benjaminpkane Looks like reverting the additional changes caused the auto-merge to be automatically disabled. My bad! Would you mind merging? All the checks have passed. |
yes, merging |
What changes are proposed in this pull request?
I corrected the install.bat script's yarn install command.
I changed it from:
to
/dev/nullis for linux, andnulis for Windows.Now the yarn install command for the fiftyone app runs in Windows, and the app can be installed from source.
I also added
/packages/app/src/components/__generated__/*to the .gitignore, as the generated files were being indexed by source control.How is this patch tested? If it is not, please explain why.
Pull the change on a Windows machine and run
install.bat -d. When running:the app will launch.
Fixes the "The system cannot find the path specified" error when running install.bat on Windows and the "Not found" message when running
fo.launch_app(dataset), as described in issue #5459.Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
Note: I've marked this as not user-facing as it only affects building from source. Please correct me if end users commonly use the install.bat script.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit