Add --legacy flag for Xcode 16 compatibility

[marinofelipe]

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 as MetadataGet and Version

Test Plan/Testing Performed:

• I didn't find tests covering the 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
• The tests pass on my machine (macOS Sonoma 14.6.1 / Xcode 16), but fail on CI - I don't have access to check which test is failing and why

[FPVMateOfficial]

Worked for me (macOS Sonoma 14.6.1 / Xcode 16)

Thanks for the quick fix!

[dailydmello]

Can this PR be reviewed and merged, as I am facing this problem aswell

[marinofelipe]

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

[tahirmt]

I think this change will break compatibility with Xcode 15. Perhaps the --legacy flag should be conditionally added instead.

[rsukumar-cpi]

Thanks for raising this PR @marinofelipe!
As mentioned by @tahirmt, the --legacy flag is not supported by older versions of xcresulttool (Xcode 15 and below). We should conditionally add the --legacy flag if the xcresulttool version being used is greater than or equal to 23028 (xcresultool version for Xcode 16 command line tools). You can look at the usage of xcresulttoolCompatibleWithUnicodeExportPath() in the Version extension as an example of a similar version check.

[abotkin-cpi]

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.

[marinofelipe]

@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.

[marinofelipe]

@abotkin-cpi , @rsukumar-cpi afad1f adds the condition. Some observations

• To resolve the condition only once I've used sort of a private global var. It could be done in other ways like a static var on the 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.
• I tested locally with both the Xcode 15.* and 16 toolchains and the logic looks correct
• No idea why the CI test failed, I can't see much besides "Xcode Test for Mac (failed)". Locally they pass on both Xcode 15.4 and 16.0. Tested on macOS 15.0 (Sequoia)

[rsukumar-cpi]

@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.

[tahirmt]

@rsukumar-cpi any idea when we can get this released?

[tahirmt]

Hi @rsukumar-cpi can you please also publish a new release with this change so it can be installed using brew?

[rsukumar-cpi]

@tahirmt @marinofelipe Just pushed a release 2.3.2 with this change. You should be able to install the latest version via brew.