PBXBuildFile objects are not strongly referenced when freshly created

[stevelandeyasana]

Context 🕵️‍♀️

I'm trying to write some code that syncs a subgroup of my project with a directory, and adds new files to the Sources phase of my build.

What 🌱

I have some code that boils down to this:

let xcp = try! XcodeProj(path: Path(xcodeprojURL.path))
let project = xcp.pbxproj.projects.first(where: { $0.name == "MyApp" })!

// am_group here is a helper that returns
// `children.first(where: { $0.path == arg }) as! PBXGroup`
let group = project.mainGroup!.am_group("DataLayerFramework").am_group("_ Models").am_group("_ Generated")

let appTarget = xcp.pbxproj.nativeTargets.first(where: { $0.name == "MyApp" })!
let sourcesPhase = appTarget.buildPhases.first(where: { $0.buildPhase == .sources }) as! PBXSourcesBuildPhase

let sourceRoot = Path(xcodeprojURL.deletingLastPathComponent().path)
for filename in directory {
  // It's safe to call this without any guards because XcodeProj dedupes file references
  let file = try! group.addFile(
      at: Path(root + "/" + filename),
      sourceRoot: sourceRoot)
  if sourcesPhase.files?.contains(where: { $0.file?.path == filename }) != true {
      print("Adding \(filename).swift to the build phase")
      let buildFile = PBXBuildFile(file: file, product: nil, settings: [:])
      sourcesPhase.files?.append(buildFile)
  }
}

try! xcp.write(path: Path(xcodeprojPath.path))

When I call xcp.write(...), I end up with this in my Sources build phase file list instead of a good references to my file:

				"TEMP_D2CF0395-3523-415D-B305-03727A331BCE" /* (null) in Sources */,

Based on my reading of PBXObjectReference and ReferenceGenerator, my guess is the file UUIDs aren't being fixed before the build UUIDs are fixed. But I'm not 100% sure of that.

Proposal 🎉

XcodeProj should fix my references correctly. :-)

[stevelandeyasana]

Bumping this, I'd love to fix this on my own but I really don't understand what's going on.

[fortmarek]

Hey @stevelandeyasana 👋

I can try looking into this but would it be possible if you first wrote a unit / integration test for this in the codebase? That would be super helpful 🙏

[stevelandeyasana]

I'll probably be able to carve out some time this week to try to break it in a test.

[stevelandeyasana]

Well, this is spooky: I wrote a test that should do exactly what I'm doing in my larger codebase, but it works. I added this to PBXProjIntegrationTests.swift:

    func test_uuids_after_adding_to_build_phase() throws {
        let fixturePath = self.fixturePath().parent().parent()
        let xcp = try XcodeProj(path: self.fixturePath().parent())
        let project = xcp.pbxproj.projects.first(where: { $0.name == "Project" })!
        let iosGroup = project.mainGroup.children.first(where: { $0.path == "iOS" }) as! PBXGroup
        let appTarget = xcp.pbxproj.nativeTargets.first(where: { $0.name == "iOS" })!
        let sourcesPhase = appTarget.buildPhases.first(where: { $0.buildPhase == .sources }) as! PBXSourcesBuildPhase

        let newFilePath = Path(components: fixturePath.components + ["iOS", "FileNotInProject.swift"])

        let file = try iosGroup.addFile(at: newFilePath, sourceRoot: fixturePath)
        let buildFile = PBXBuildFile(file: file, product: nil, settings: [:])
        sourcesPhase.files?.append(buildFile)

        let encoder = PBXProjEncoder(outputSettings: PBXOutputSettings())
        let output: String = try encoder.encode(proj: xcp.pbxproj)
        XCTAssert(!output.contains("TEMP"))
    }

It passed. :-( I'll keep poking at it.

Differences I can think of vs my own project file:

• More build targets
• More build phases
• I'm dynamically iterating over files in a directory instead of using a static path

[stevelandeyasana]

I tried attaching a debugger to see if I could see where ReferenceGenerator skips over my new files. I found that when it's iterating over the build file references, it's skipping at least one because it can't find the PBXBuildFile that goes with a PBXObjectReference. I'm still trying to work out what that means. These conditions do happen when working with my main project, and not with the integration test I wrote above.

    private func generateBuildPhaseReferences(_ buildPhase: PBXBuildPhase,
                                              identifiers: [String]) throws {
        var identifiers = identifiers
        if let name = buildPhase.name() {
            identifiers.append(name)
        }

        // Build phase
        fixReference(for: buildPhase, identifiers: identifiers)

        // Build files
        buildPhase.fileReferences?.forEach { buildFileReference in
            if !buildFileReference.temporary { return }

            // THIS LINE: buildFileReference.getObject() returns nil
            guard let buildFile: PBXBuildFile = buildFileReference.getObject() else { return }

[stevelandeyasana]

OK, this looks like a spooky memory management thing. If I make an [Any] array outside my loop, and append the PBXBuildFiles I create to that array, then it works. My example integration test doesn't have a loop, so it doesn't have the bug. When I add the loop to the integration test, it fails. I'll open a PR with the failing test, and fix my code using this workaround.

[stevelandeyasana]

Clearer explanation of what I learned: XcodeProj does not keep any strong internal references to PBXBuildFile, so unless the caller keeps its own references, newly created PBXBuildFile objects added to a build phase may be come nil before the project file is written.

I think this is really surprising behavior, but it does have a straightforward workaround.

[fortmarek]

XcodeProj does not keep any strong internal references to PBXBuildFile, so unless the caller keeps its own references, newly created PBXBuildFile objects added to a build phase may be come nil before the project file is written.

Ouch, that's a pretty good find!

It does seem to be the case:

• PBXObjectReference keeps a weak pointer to its objects (PBXBuildFile will be one of those)
• PBXBuildPhase does not store PBXBuildFiles directly - instead uses the getter in PBXObjectReference

We'll need to keep a strong pointer somewhere.

I think we can rewrite:

    /// References to build files.
    var fileReferences: [PBXObjectReference]?

    /// Build files.
    public var files: [PBXBuildFile]? {
        get {
            fileReferences?.objects()
        }
        set {
            newValue?.forEach { $0.buildPhase = self }
            fileReferences = newValue?.references()
        }
    }

to:

    /// References to build files.
    var fileReferences: [PBXObjectReference]? {
          files?.references()
    }

    /// Build files.
    public var files: [PBXBuildFile]? {
        didSet {
            files?.forEach { $0.buildPhase = self }
        }
    }

This change will strongly hold the PBXBuildFiles and subsequently also the PBXObjectReferences since the former holds strongly the latter but not the other way around.

Would such a change make sense @stevelandeyasana? If so, let me know if you wanted to implement this yourself or I can go ahead. Either way, it would be great if you tested it in your project where you have first seen the issue.

[stevelandeyasana]

Unfortunately that isn't quite sufficient. The initializers directly assign to fileReferences, which you changed from a get/set to just a get property, and I'm not sure what's supposed to happen with the Decodable initializer. But otherwise that looks like a good solution.

[github-actions]

Hola 👋,

We want to inform you that the issue has been marked as stale. This means that there hasn't been any activity or updates on it for quite some time, and it's possible that it may no longer be relevant or actionable.
If you still believe that this issue is valid and requires attention, please provide an update or any additional information that can help us address it. Otherwise, we may consider closing it in the near future.
Thank you for your understanding.

[github-actions]

Hola 👋,

We want to inform you that we have decided to close this stale issue as there hasn't been any activity or response regarding it after marking it as stale.

We understand that circumstances may have changed or priorities may have shifted, and that's completely understandable. If you still believe that this issue needs to be addressed, please feel free to reopen it and provide any necessary updates or additional information.

We appreciate your understanding and look forward to your continued contributions to the project.

Thank you.