Closed marinofelipe closed 1 month ago
Worked for me (macOS Sonoma 14.6.1 / Xcode 16)
Thanks for the quick fix!
Can this PR be reviewed and merged, as I am facing this problem aswell
Can this PR be reviewed and merged, as I am facing this problem aswell
@abotkin-cpi could you please take a look? Let me know if there's anything missing, such as extending the tests. Thank you
I think this change will break compatibility with Xcode 15. Perhaps the --legacy
flag should be conditionally added instead.
Hey @marinofelipe, sorry I've been out this last month for my wedding & honeymoon. As @tahirmt mentioned and @rsukumar-cpi from our ChargePoint team confirmed in testing, we'll need to add backwards compatibility support prior to merging due to the limitations of older xcresulttool. If you have the chance to add it, that would be awesome, otherwise I'll take a stab at adding it to your PR when I'm free this weekend.
@tahirmt , @rsukumar-cpi , you're correct. I had a note to ask if backwards compatibility was needed, or if the next release could be exclusive to the Xcode 16 toolchain, but completely forgot it 🤦♂️ , thanks for the heads up and the pointer regarding how similar version checks are done.
Hey @marinofelipe, sorry I've been out this last month for my wedding & honeymoon. As @tahirmt mentioned and @rsukumar-cpi from our ChargePoint team confirmed in testing, we'll need to add backwards compatibility support prior to merging due to the limitations of older xcresulttool. If you have the chance to add it, that would be awesome, otherwise I'll take a stab at adding it to your PR when I'm free this weekend.
Thanks @abotkin-cpi . I'll try to go and add myself early this weekend, but if I don't get to it then please do.
@abotkin-cpi , @rsukumar-cpi afad1f adds the condition. Some observations
XCResultToolCommand
type, etc. I used the approach I found cleaner, but let me know in case you have other preferences, or feel free to do changes/commit on top of it. For example, I didn't worry much about testability, injecting it, etc.@marinofelipe The changes look good to me. I'm looking into the failing tests in our CI right now. Once the issue is resolved, I will merge this PR.
@rsukumar-cpi any idea when we can get this released?
Hi @rsukumar-cpi can you please also publish a new release with this change so it can be installed using brew
?
@tahirmt @marinofelipe Just pushed a release 2.3.2 with this change. You should be able to install the latest version via brew
.
Change Description:
As discussed in #87. This is a short term solution, for a long term one the
xcresulttool
usage is to be later reviewed.I didn't simply change
XCResultToolCommand:13
to include--legacy
because some commands don't require and fail when it gets added, such asMetadataGet
andVersion
Test Plan/Testing Performed:
xcresulttool
command that is generated, to extend asserting for the presence of--legacy
, but please let me know if there's one or if I should write a test for it