fix getxcodeversion usage #996
Conversation
services/xcode-select-service.ts
Outdated
| let xcodeVer = await sysInfoBase.getXCodeVersion(); | ||
| if (!xcodeVer) { | ||
| this.$errors.fail("xcodebuild execution failed. Make sure that you have latest Xcode and tools installed."); | ||
| } |
There was a problem hiding this comment.
Can you add one new line after this }
sys-info-base.ts
Outdated
| } | ||
|
|
||
| return this.xCodeVerCache; | ||
| return this.$hostInfo.isDarwin ? await this.exec("xcodebuild -version") : null; |
There was a problem hiding this comment.
Why we don't want to have cache anymore?
There was a problem hiding this comment.
i'll return the cache
sys-info-base.ts
Outdated
| @@ -157,9 +147,9 @@ export class SysInfoBase implements ISysInfo { | |||
| res.procArch = process.arch; | |||
| res.nodeVer = process.version; | |||
|
|
|||
| res.npmVer = await this.getNpmVersion(); | |||
| res.npmVer = await this.getNpmVersion(); //not used anywhere except for tests | |||
There was a problem hiding this comment.
Can you check if we use this in the AppBuilder CLI
There was a problem hiding this comment.
i got no private access to look at the repo, i'll remove the comments
sys-info-base.ts
Outdated
|
|
||
| res.javaVer = await this.getJavaVersion(); | ||
| res.javaVer = await this.getJavaVersion(); //not used anywhere except for tests |
There was a problem hiding this comment.
Can you check if we use this in the AppBuilder CLI
There was a problem hiding this comment.
i got no private access to look at the repo, i'll remove the comments
sys-info-base.ts
Outdated
| @@ -207,6 +197,7 @@ export class SysInfoBase implements ISysInfo { | |||
| } | |||
| } catch (e) { | |||
| // if we got an error, assume not working | |||
| this.$logger.trace(`Error while executing child process: ${e}`); | |||
There was a problem hiding this comment.
You can change child process to ${cmd}. Also I think if we log ${e} here, in the logs we will see [object object] instead of the error. You can log e.message.
Plamen5kov
force-pushed
the
plamen5kov/fix-getxcodeversion-usage
branch
from
423ea30 to
20888cf
Compare
services/xcode-select-service.ts
Outdated
| xcodeVersionMatch = xcodeVer.match(/Xcode (.*)/), | ||
| let xcodeVer = await sysInfoBase.getXCodeVersion(); | ||
| if (!xcodeVer) { | ||
| this.$errors.fail("xcodebuild execution failed. Make sure that you have latest Xcode and tools installed."); |
There was a problem hiding this comment.
Can we use this.$errors.failWithoutHelp here or we need to use fail?
There was a problem hiding this comment.
I originally made it to failWithoutHelp, but for consistency with https://github.com/NativeScript/nativescript-cli/blob/1955028ac3ded6a012813476f08d0f4f0d89dddd/lib/services/ios-project-service.ts#L1190 i
If you insist I have no objection of making it fail without help. I just want to have the same behavior in the code.
There was a problem hiding this comment.
I think it is better to use this.$errors.failWithoutHelp in the services.
|
This code will not fix the mentioned issue, it will just fail with different error when executing @Plamen5kov am I missing something here, maybe there's another PR in {N} CLI that would fix the |
|
@rosen-vladimirov I thought failing was the expected behavior, my bad. I'll fix it in another PR. |
|
@Plamen5kov the failure in the current code (changed in this PR) is fine IMO. But |
|
Yes, my plan was to add a check to |
problem
related issue: NativeScript/nativescript-cli#2810
getxcodeversionreturning null isn't handled correctly intns doctorsolution
handle erroneous case and remove unnecessary code