Improve INFOPLIST_FILE handling to only omit used Info.plist's from Copy Bundle Resources Build Phase

[liamnichols]

Closes #1045

Background

Prior to #945, a lot of XcodeGen was build assuming that a file named Info.plist was the value used in INFOPLIST_FILE build setting. This file is different to other plists because it shouldn't be included in the copy bundle resources build phase.

If INFOPLIST_FILE was named anything other than Info.plist, by default, XcodeGen would include the file in the build phase which would result in a warning.

warning: The Copy Bundle Resources build phase contains this target's Info.plist file 'HardwareSupport-Info.plist'. (in target 'HardwareSupport' from project 'LegacySupport')

#945 took a stab at fixing this by relaxing the checks so that any file that had been suffixed with Info.plist would then be treated as an Info.plist to support the old(?) pattern of using a file named {TargetName}-Info.plist. This however caused a noticeable regression for projects that include the GoogleService-Info.plist file which should be treated as a resource (this file is nothing to do with an bundles info property list and instead is a resource used by popular Google SDKs.

So I looked back at @yonaskolb's initial comment:

but a proper fix might be to check the build settings of the target to find all the target's info plists and exclude them.

Description

In this change, I am taking a stab at improving things for both scenarios. To begin with in 5a856b8, I checked out 2.18.0 and added a GoogleService-Info.plist file to the resources and regenerated the fixtures as expected. I then went back to 2.19.0 and also generated the fixtures with a target which defines the Info.plist as App-Info.plist in
61e2c3b.

Now that we have updated fixtures that reproduce both use cases, it should now be a matter of changing things until the tests pass 😬

In PBXProjGeneractor, the final INFOPLIST_FILE build setting value for a given target isn't resolved until wayyyy after the array of SourceFile objects (which define the desired buildPhase) are created. I figured that the first task would be to resolve these values upfront so that we can use the actual INFOPLIST_FILE values to influence the SourceFile's that we resolve (instead of guessing based on filename). So far, I've refactored the build setting resolution logic in 56ef412e6e87431a33ab71470a6ca130a1ac27c4 so that I now have a [Config: String?] dictionary that contains the INFOPLIST_FILE values for each build config of the target.

What's Next

The next step is to somehow pass this information into SourceGenerator.getAllSourceFiles(targetType:sources:) so that it'll return a SourceFile with it's buildPhase value set to BuildPhaseSpec.none for the real Info.plist files (to make the tests pass). This is where I'm a little stuck.

After trying to poke around, I felt like it was quite difficult to pass the set of files deep into the SourceGenerator and before I continue, I want to confirm that I'm on the right tracks.

Part of me did wonder if I could maybe instead just mutate the target.sources array before passing it into the SourceGenerator since I could just pretend that it was just set in the spec file, but I wondered if it was wrong to override the value that was potentially set in the project spec in doing this. Ideally we should probably prioritise the explicitly set value before and just use .none as a fallback before the result of getDefaultBuildPhase(for:targetType:).

I've run out of time this evening on this one so before I look at it with a fresh pair of eyes later this weekend, I thought I'd push the changes up and maybe somebody else could give some input (👀 @yonaskolb 😬)

[yonaskolb]

Thanks so much for looking at this @liamnichols, I like the way you're approaching it so far

[liamnichols]

@yonaskolb: I hit a bit of a bump here while trying to think things through throughly...

In theory, the value of the INFOPLIST_FILE build setting could be something like ${SRCROOT}/SupportingFiles/${CONFIG_NAME}-Info.plist and if this is the case, we'd have no way to be able to figure that one out how that maps to a SourceFile to be able to influence the buildPhase that is assigned.

While I think it's probably uncommon to change the filename based on random build settings, I don't think it's unrealistic for people to have carried over SRCROOT or concepts like that as a way to use a path relative to the project root.

These are the options that I came up with:

1. Just assume that if the user defined INFOPLIST_FILE in their spec, that the value is relative to the project root and does not include any variables that require expansion and accept that behaviour will be inconsistent.
2. Attempt to expand any variables - But I think that this is impossible because we don't have the compile-time build settings?
3. Only automatically alter the build phase in the following circumstances:
   i. If the user defined the info.path property in the spec
   ii. If the user neither defined info.path OR INFOPLIST_FILE and we instead XcodeGen automatically found the file called exactly Info.plist in the targets sources.

Option 3 would basically mean that if you defined INFOPLIST_FILE in your project spec and your info property list was called something other than Info.plist then you would have to explicitly stop that .plist from being treated as a bundle resource. I think that this is the most sensible option, but I'm interested in feedback.

Either way, we would probably have to document this a bit better somewhere.

[yonaskolb]

@liamnichols. I think we want to optimise for a working project. At the end of the day, if the Info.plist file is bundled as a resource it's not a major deal, and there will always be a way to remove this by explicitly setting all those files to resource none. So we should use our best efforts to exclude it, but within reason. Option 2 is something I don't want to handle. I think option 1 is fine. We'll check against the path, and if that includes build variables it just won't match and won't be excluded.

[liamnichols]

Before I started trying to pass the phases in, I found a couple of method arguments that seemed to be redundant in SourceGenerator and did a little cleanup in 7dd55fa5b91fb0166276034073e843b0cc7c146d

getSourceFiles(targetType:targetSource:path:)

This method accepted a path value but it was always project.basePath + targetSource.path so seemed redundant injecting it given that we could compute the value inside the method.

Doing this also avoided a bit of confusion where parts of the method used path and other parts declared a local folderPath argument which was also the same - let folderPath = project.basePath + Path(targetSource.path).

I might have missed the initial intentions of injecting the path argument, but since its redundant today, it seems ok to remove it.

generateSourceFile(targetType:targetSource:path:buildPhase:fileReference:)

This method was my main target during refactoring. It accepted a buildPhase property (optional, nil) that if set, was used as a priority over the value specified as targetSource.buildPhase.

It turns out that this argument was only ever used in one of the calls to this method, and instead of using this value as an override, the intention was that actually this value was to be used as a fallback in the event hat targetSource.buildPhase had not been set with had resulted in some duplicated code.

To work around this, I removed the argument and actually just incorporated the use internally within the method by refactoring some common logic into resolvedTargetSourceType(for:at:) in order to override the buildPhase to be .resources when the SourceType is .folder. The reasoning behind this should hopefully be clear in the diff of this commit specifically

---

I'm relying on the fact that existing tests continue to pass here and I'm pretty confident that these changes do not change the behaviour in any way, but if you want some more test coverage for confirmation then please let me know 🙇

[yonaskolb]

Looks good to me @liamnichols. Thanks for cleaning this up. This file and the PBXGenerator have had all sorts of hands in it changing different things, so they need a bit of love 😄

[liamnichols]

@yonaskolb: I've now finished the implementation with 06bd547 to get the tests into a passing state. Again, feedback on the approach would be super appreciated 🙏 It was simpler than I was expecting but I wonder if I need to take extra care around resolving paths?

I could probably do with adding some extra tests to SourceGeneratorTests.swift to catch some edge cases but just wondering how much to add.

Cheers!

[yonaskolb]

Looks good @liamnichols!

[yonaskolb]

@liamnichols whenever you have time to update this, it would be good to release this, as it's a fix in some respects

[liamnichols]

Ah thanks for reminding me. I’ll have some time over this weekend to get it ready 👍

[liamnichols]

Things left to do:

• Rebase from master
• Update CHANGELOG.md
• Move the call to Path.absolute() into getInfoPlist(for:)
• Add some extra unit tests to SourceGeneratorTests.swift
• Clarify behaviour in docs if needed

[liamnichols]

• Clarify behaviour in docs if needed

Not sure if i need to do anything here actually. Let me know what you think @yonaskolb.

This is ready now 👍

[yonaskolb]

Looks good! Thank you again for your fantastic work here @liamnichols! 👏