xcparse icon indicating copy to clipboard operation
xcparse copied to clipboard

Add `--legacy` flag for Xcode 16 compatibility

Open marinofelipe opened this issue 1 year ago • 8 comments

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

marinofelipe avatar Sep 13 '24 08:09 marinofelipe

Worked for me (macOS Sonoma 14.6.1 / Xcode 16)

Thanks for the quick fix!

FPVMateOfficial avatar Sep 17 '24 23:09 FPVMateOfficial

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

dailydmello avatar Oct 03 '24 18:10 dailydmello

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

marinofelipe avatar Oct 05 '24 13:10 marinofelipe

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

tahirmt avatar Oct 10 '24 18:10 tahirmt

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.

abotkin-cpi avatar Oct 10 '24 23:10 abotkin-cpi

@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 avatar Oct 11 '24 11:10 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)

marinofelipe avatar Oct 12 '24 14:10 marinofelipe

@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 avatar Oct 14 '24 20:10 rsukumar-cpi

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

tahirmt avatar Oct 24 '24 03:10 tahirmt

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

tahirmt avatar Oct 28 '24 14:10 tahirmt

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

rsukumar-cpi avatar Oct 28 '24 22:10 rsukumar-cpi