Shell Script Build Phases are not properly compared

[michaelmcguire]

Describe the bug Comparisons of build phases does not compare all properties for script actions.

To Reproduce Steps to reproduce the behavior:

1. Clone the repo
2. cd Fixtures
3. open Project1.xcodeproj
4. Add "New Run Script Phase" with an empty script.
5. swift run xcdiff --target Project --tag BUILD_PHASES -v shows difference:

   
   ❌ BUILD_PHASES > "Project" target

⚠️ Only in first (1):

• ShellScript (runScript)

6. `open Project2.xcodeproj`
7. Add "New Run Script Phase" with an empty script.
8. `swift run xcdiff --target Project --tag BUILD_PHASES -v` outputs: 

✅ BUILD_PHASES > "Project" target

9. Add a new default "Input File Lists" item to `Project1`: `$(SRCROOT)/newInputFile.xcfilelist`
10. `swift run xcdiff --target Project --tag BUILD_PHASES -v` shows difference: 

❌ BUILD_PHASES > "Project" target

⚠️ Value mismatch (1):

• Different properties in "ShellScript (runScript)" ◦ inputFileListPaths = ["$(SRCROOT)/newInputFile.xcfilelist"] ◦ inputFileListPaths = []

11. Remove `$(SRCROOT)/newInputFile.xcfilelist` from "Input File Lists" in `Project1` and try a different setting like "Input Files" or "Show environment variables in build log". Changing other properties in the build phase results in:

✅ BUILD_PHASES > "Project" target

**Expected behavior**
Display the differences in the build phase beyond the "Input File Lists"

**Screenshots**
<img width="838" alt="image" src="https://user-images.githubusercontent.com/429790/186781079-2196ed7a-1701-422c-ba8a-1bfa01b3ae7e.png">

**Environment (please complete the following information):**
 - Operating System and Version: macOS 12.4
 - `xcdiff` version: `main`

**Additional context**
*Most* of the settings in a ShellScript/Run Script action are _not_ compared when running `xcdiff`. Looking [at the properties compared](https://github.com/bloomberg/xcdiff/blob/main/Sources/XCDiffCore/Comparator/BuildPhasesComparator.swift#L163-L175), it looks as if only the base, abstract class `PBXBuildPhase` is being used for building the descriptor. The actual type that's returned back for script phases is `PBXShellScriptBuildPhase` and it contains all of the properties for script phases.

I'm working on a fix for this, but wanted to verify that 1. this is considered a bug, and 2. showing these differences in the `BUILD_PHASES` section is appropriate. I'm planning on looking at the type returned for each `PBXTarget.buildPhases` and if I see a `PBXShellScriptBuildPhase`, comparing all of the properties there as well.

Thanks for this amazing tool! It's been very helpful.

[kwridan]

Thanks for raising this @michaelmcguire and for the thorough details and context 👍

I'm working on a fix for this

Thank you, this would be a welcome contribution!

1. this is considered a bug

It's something we missed when working, but would be great to have xcdiff display the diff for those properties too.

2. showing these differences in the BUILD_PHASES section is appropriate.

We do have a dedicated comparator for PBXCopyFilesBuildPhase which is also a build phase, for consistency perhaps we can have a dedicated comparator for script phases too?

@marciniwanicki would it be odd to have duplicate or partial information for the same build phase under two different comparators? e.g. inputFileListPaths appears under BUILD_PHASES but the new comparator would display the diff of the other properties such as "Based on dependency analysis" (alwaysOutOfDate).

[michaelmcguire]

It's something we missed when working, but would be great to have xcdiff display the diff for those properties too.

Great! Thanks for the verification, @kwridan.

2. showing these differences in the BUILD_PHASES section is appropriate.

We do have a dedicated comparator for PBXCopyFilesBuildPhase which is also a build phase, for consistency perhaps we can have a dedicated comparator for script phases too?

@marciniwanicki would it be odd to have duplicate or partial information for the same build phase under two different comparators? e.g. inputFileListPaths appears under BUILD_PHASES but the new comparator would display the diff of the other properties such as "Based on dependency analysis" (alwaysOutOfDate).

Yeah, I noticed that several of the build phases (dependencies, resources) have dedicated comparators and listed themselves separately from BUILD_PHASES. It seems to me like having a separate place for these differences to be reported would make sense as well.

I have an issue opened with XcodeProj to make accessing the script phases easier (with a PR to be opened today), but it's not strictly necessary to fix this issue (which I'll also be working on today).

Let me know if you'd like it as a new report section or done inside of BUILD_PHASES. I'll probably move forward for now implementing as a new comparator but if you'd like it done inside BUILD_PHASES, shouldn't be too difficult to change.

Thanks for your quick response!

[kwridan]

Thank you for the follow up @michaelmcguire

I have an https://github.com/tuist/XcodeProj/issues/716 to make accessing the script phases easier (with a PR to be opened today), but it's not strictly necessary to fix this issue (which I'll also be working on today).

awesome, will review that shortly.

Let me know if you'd like it as a new report section or done inside of BUILD_PHASES. I'll probably move forward for now implementing as a new comparator but if you'd like it done inside BUILD_PHASES, shouldn't be too difficult to change.

Makes sense having it as a separate comparator for consistency - perhaps called with a tag of scripts ?

[michaelmcguire]

Thank you for the follow up @michaelmcguire

I have an tuist/XcodeProj#716 to make accessing the script phases easier (with a PR to be opened today), but it's not strictly necessary to fix this issue (which I'll also be working on today).

awesome, will review that shortly.

Thanks!

Let me know if you'd like it as a new report section or done inside of BUILD_PHASES. I'll probably move forward for now implementing as a new comparator but if you'd like it done inside BUILD_PHASES, shouldn't be too difficult to change.

Makes sense having it as a separate comparator for consistency - perhaps called with a tag of scripts ?

That works. I was thinking of generally referring to it as "run script" everywhere since that is how it's defined in the UI, but I can just shorten to scripts (or whatever else you like when I open the PR).

[kwridan]

That works. I was thinking of generally referring to it as "run script" everywhere since that is how it's defined in the UI, but I can just shorten to scripts (or whatever else you like when I open the PR).

run_scripts works too 👍

[kwridan]

Resolved via: https://github.com/bloomberg/xcdiff/pull/112