feat(apple): Fix doctor issue with asdf - #6062
Conversation
If the user is using ASDF the 'doctor' command may fail at the Cocoapods stage as it will be using the global tool definitions instead of those specified by the project (if any). This fixes the problem by generating a .tool-versions file with the correct settings in the temp directory used for the Cocoapods test.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesASDF Ruby Support in CocoaPods Verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/doctor/src/sys-info.ts`:
- Around line 429-444: The asdf probe in the spawnFromEvent call at line 429
unconditionally fails if asdf is missing or misconfigured, causing healthy
setups to incorrectly report as broken. Additionally, using asdf list ruby may
match a non-active version instead of the currently active one. Add ignoreError:
true option to the spawnFromEvent call to prevent throwing, change the asdf
command from "list ruby" to "current ruby" to explicitly resolve the active
project Ruby version (which outputs the specific active version and
configuration source), and update the version matching logic if needed to work
with the output format of asdf current ruby.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4a87b1a9-ab68-4240-9fb0-55de6c4fd509
📒 Files selected for processing (2)
packages/doctor/src/sys-info.tspackages/doctor/src/wrappers/file-system.ts
Fix error handling in asdf invocation.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/doctor/src/sys-info.ts (1)
447-450:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTerminate the appended
.tool-versionsentry with a newline.At Line 449, appending
ruby ${asdfVersion}withoutEOLcan merge with existing content in.tool-versionsand produce an invalid line when the file is pre-populated.Suggested patch
const wroteASDFConfig = this.fileSystem.appendFile( asdfConfigPath, - `ruby ${asdfVersion}`, + `ruby ${asdfVersion}${EOL}`, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/doctor/src/sys-info.ts` around lines 447 - 450, The appendFile call in the asdfConfigPath section is appending `ruby ${asdfVersion}` without a trailing newline character, which will cause the content to merge with existing entries in the pre-populated `.tool-versions` file and create invalid lines. Add a newline character (EOL or \n) at the end of the template string being appended in the appendFile call for asdfConfigPath to ensure each entry is properly terminated and on its own line.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/doctor/src/sys-info.ts`:
- Around line 447-450: The appendFile call in the asdfConfigPath section is
appending `ruby ${asdfVersion}` without a trailing newline character, which will
cause the content to merge with existing entries in the pre-populated
`.tool-versions` file and create invalid lines. Add a newline character (EOL or
\n) at the end of the template string being appended in the appendFile call for
asdfConfigPath to ensure each entry is properly terminated and on its own line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 39b2db4a-fa53-4b30-b4f8-87d7f248a641
📒 Files selected for processing (1)
packages/doctor/src/sys-info.ts
| ["current", "ruby"], | ||
| "exit", | ||
| { ignoreError: true }, | ||
| ); |
There was a problem hiding this comment.
This should set cwd since ns doctor can be run outside of project directories.
| ); | ||
| const wroteASDFConfig = this.fileSystem.appendFile( | ||
| asdfConfigPath, | ||
| `ruby ${asdfVersion}`, |
There was a problem hiding this comment.
| `ruby ${asdfVersion}`, | |
| `ruby ${asdfVersion}\n` |
NathanWalker
left a comment
NathanWalker
left a comment
There was a problem hiding this comment.
Just two nit's on version resolution being cwd explicit, a trailing newline for the tools etiquette (ensuring other appends don't mash together), and if possible to add a small unit test around asdf-present/asdf-absent behavior would be 💯 thank you!
|
@apburgess possible to enable 'Allow edit from maintainers' on your fork? I could push small adjustment to get this merged 👍 |
|
See this as reference to adjustment: https://github.com/NativeScript/nativescript-cli/pull/6086/changes - you can make here if you'd like and happy to merge yours instead. |
|
@NathanWalker Apologies, I was off on another project and kept meaning to get back to this; I've added 'Allow edits from maintainers' but #6086 seems to cover the outstanding bits, thanks! |
Resolve the asdf Ruby version with an explicit cwd so it reflects the project even when `ns doctor` is invoked outside the project directory, and newline-terminate the `.tool-versions` entry so it cannot merge with existing content. Adds unit tests for the asdf-present and asdf-absent paths.
|
Excellent thank you @apburgess, pushed up small nit's and this can proceed now 💯 |
2 participants
PR Checklist
What is the current behavior?
Currently iOS builds can fail in the
ns doctorstage when usingasdfto specify the Ruby version; this happens because the CocoaPods checks performed byns doctorare done in a new directory in /tmp which is missing theasdfconfiguration.What is the new behavior?
If
asdfis installed, the Ruby version specified for the project is added to a.tool-versionsfile in the CocoaPods test project folder; this stops thens doctorcommand from failing and lets the build continue.Implements #6061.
Note: I have run the existing tests; the only test that fails is 'skip when missing hook args' - but this also fails on a clean clone from the repo.
Summary by CodeRabbit
.tool-versionsupdates.