Introduce XcodeProjToGraph and TestSupport Modules for Workspace/Project Mapping #87
Conversation
ajkolean
changed the title
There was a problem hiding this comment.
First of all, thanks a lot @ajkolean 👏
This work is huge and will enable powerful workflows that the Swift community has not seen, yet.
I did a first pass, trying to mostly focus on the higher level structure of the codebase.
Some of the comments are applicable across the PR, so it'd be amazing if you could have that in mind as I didn't want to repeat myself too much. I tried to point out when I saw some pattern we should imho redo in other places as well, but I might not have done this perfectly.
Overall, I am very onboard with the structure of breaking down the mapping of XcodeProj into multiple layers of individual mappers that have a simple responsibility.
Some of the patterns I'd consider changing (and these are also captured in the individual review comments):
- Prefer structs over classes
- Minimize the public interface of the
XcodeProjToGraphmodule to the bare minimum. This will help us keep this repository stable. - Don't overdo it with running things in parallel. Benchmark first and make asynchronous only parts of the code that are actually resource intensive.
- Use our utility for running shell commands instead of having a new implementation: https://github.com/tuist/command
- Utilities should usually be configured directly via methods instead of passing the configuration at the
inittime. Unless there's a clear win by configuration utilities throughinit, let's keepinitreserved for dependency injection. - Individual mappers should not be accessing layers above. For example, a
PBXBuildPhasemapper should not have a notion ofPBXTarget.
Let me know if we can help you in any way to proceed. This is top on my priority list, so I'll try to provide feedback swiftly here as you go through the review.
Once higher-level concerns are resolved, I'll do another pass going deeper into the individual implementations as I skimmed through some of them during the initial pass.
This must have been a ton of work and we really appreciate it – this is open source at its best 💜
Sources/XcodeProjToGraph/Mappers/Settings/ConfigurationMatcher.swift
Outdated
Show resolved
Hide resolved
Sources/XcodeProjToGraph/Mappers/TargetAndSchemes/TargetMapper.swift
Outdated
Show resolved
Hide resolved
| /// - Throws: | ||
| /// - `ProcessRunnerError.failedToRunProcess` if `lipo` exits with a non-zero code. | ||
| /// - Any errors related to decoding or interpreting the output as architectures. | ||
| public func parseLipoArchsResult(_ result: ProcessResult) throws -> LipoArchsResult { |
There was a problem hiding this comment.
Have you considered using the implementation that we have in tuist: https://github.com/tuist/tuist/blob/main/Sources/TuistCore/MetadataProviders/FrameworkMetadataProvider.swift ?
I believe we can use it instead. If so, then I'd consider creating a new module called something like ProductMetadataProvider or BinaryMetadataProvider that we can then share back with tuist instead of having the implementation duplicated. If we end up doing this, I'd consider moving the whole MetadataProviders directory into XcodeGraph: https://github.com/tuist/tuist/tree/main/Sources/TuistCore/MetadataProviders
There was a problem hiding this comment.
That’s a great callout. Thanks for pointing this out! I’ve added TODO comments to explore how best to align with this or where it should live. It might not be as straightforward as dropping it in, since the FileHandler here already uses TuistSupport (though only for the Info.plist). That said, I think this could be a great opportunity to modularize FileHandling or maybe more as a whole, especially if we can align with the broader patterns in Tuist.
There was a problem hiding this comment.
The FileHandler should be removed in favor of FileSystem. It should be already possible. If that's the only blocking thing to move it here, then we should be good.
Tests/XcodeProjToGraphTests/MapperTests/Xcode/Build/BuildRuleMapperTests.swift
Outdated
Show resolved
Hide resolved
|
@fortmarek I’ve gone through and addressed almost everything: cleaned up the mappers, streamlined the test data, and tightened up public access. If there’s anything I missed or further tweaks you’d suggest, I’m all ears. Thanks again for taking the time to go through this so thoughtfully. Your sharp eye and collaborative approach made this a great process. Looking forward to any more thoughts! 🙏 |
There was a problem hiding this comment.
Some additional feedback. Mostly nits. Some bigger pieces of feedback from this round I'd like to call out:
- When we run into unknown objects and scenarios, I'd lean into throwing instead of swallowing these objects as
nil. That will make it easier for us to spot our blinds spots. Otherwise, the conversion might be lossier than one would expect and then it's gonna be hard to figure out why. - I'd consider removing
ProjectParserand makingGraphMapperas the public interface. To do that, we'll need to make a PR inXcodeProjto accept anAbsolutePathin itsinit - Let's also update
XcodeProjto hold its ownpath– I'd like us to move away from theProjectProviderandWorkspaceProviderhelpers and useXcodeProjdirectly.
| public static func test( | ||
| static func test( |
There was a problem hiding this comment.
We actually use these in tuist/tuist – this would be a breaking change. We could have a debate on whether this is a good pattern or not, but for the purposes of this PR, I'd keep these test extension in XcodeGraph as public.
| extension AbsolutePath { | ||
| /// Maps a path by its extension to a `TargetDependency` if applicable. | ||
| /// | ||
| /// - Parameter condition: Optional platform condition. | ||
| /// - Returns: A `TargetDependency` if the extension matches known dependency types. | ||
| func mapByExtension(condition: PlatformCondition?) -> TargetDependency? { |
There was a problem hiding this comment.
I'd move this to its own mapper that follows the structure of other mappers. Something akin to:
struct PathDependencyMapper: PathDependencyMapping {
func map(_ pathDependency: AbsolutePath) -> TargetDependency? { ... }
}| case let .developer(subPath): | ||
| return try AbsolutePath( | ||
| validating: subPath, | ||
| relativeTo: try AbsolutePath(validating: "/Applications/Xcode.app/Contents/Developer") |
There was a problem hiding this comment.
This is assuming a given Xcode path which might be different.
You'll need to do something similar to: https://github.com/tuist/tuist/blob/main/Sources/TuistSupport/Xcode/XcodeController.swift
I'd use a more simplified version of XcodeController. We'll need to use a CommandRunner, so this method will need to be asynchronous.
There was a problem hiding this comment.
I'd try to remove this and expose the GraphMapper instead that will take an XcodeProj as an input. To do that, we'll need to update XcodeProj model to hold on to its own path and thus will require a PR in that repo – but I think it's worth doing. That way, we might be able to get rid of the ProjectProvider and WorkspaceProvider pattern.
I imagine the usage of the library something like:
import XcodeProj
import XcodeGraphMapper
let myPath = try AbsolutePath(validating: "/xcode-path")
let xcodeProj = XcodeProj(path: myPath)
let xcodeGraph = XcodeGraphMapper().map(xcodeProj)
...I'd also update the init of XcodeProj to accept AbsolutePath from our utility library. We'll need to internally convert it to the PathKit implementation.
| init( | ||
| graphType: GraphType, | ||
| projectProviderClosure: @escaping (AbsolutePath) throws -> ProjectProviding = { path in | ||
| let xcodeProj = try XcodeProj(pathString: path.pathString) | ||
| return ProjectProvider(xcodeProjPath: path, xcodeProj: xcodeProj) | ||
| } | ||
| ) { | ||
| self.graphType = graphType | ||
| self.projectProviderClosure = projectProviderClosure | ||
| } |
There was a problem hiding this comment.
Instead of using init, let's pass these through the map method. I'd also update the GraphMapper interface to take in the XcodeProj and figure out the type by itself:
struct GraphMapper: GraphMapping {
func map(_ xcodeProj: XcodeProj) -> XcodeGraph { ... }
}| guard result.isEmpty else { return result } | ||
|
|
||
| // Fallback if no platform detected. | ||
| let product = productType?.mapProductType() ?? .app |
There was a problem hiding this comment.
Should we consider throwing instead of falling back to .app?
| } | ||
| } | ||
|
|
||
| return nil |
| // Direct reference to another target in the same project. | ||
| return .target(name: remoteInfo, status: .required, condition: condition) | ||
| case let .fileReference(fileReference): | ||
| guard let projectRelativePath = fileReference.path else { return nil } |
| // Reference to a target in another project. | ||
| return .project(target: remoteInfo, path: absPath, status: .required, condition: condition) | ||
| case .unknownObject: | ||
| return nil |
| ) | ||
| } | ||
|
|
||
| // MARK: - Helpers |
There was a problem hiding this comment.
Let's mark the helpers as private
| @@ -3,6 +3,7 @@ import ProjectDescription | |||
|
|
|||
| public enum Module: String, CaseIterable { | |||
| case xcodeGraph = "XcodeGraph" | |||
| case XcodeProjMapper | |||
There was a problem hiding this comment.
| case XcodeProjMapper | |
| case xcodeProjMapper |
Nit: I'd preserve camelCasing here.
| ), | ||
| mainGroup: PBXGroup, | ||
| targets: [PBXTarget] = [PBXNativeTarget.test()], | ||
| schemes _: [XCScheme] = [] |
There was a problem hiding this comment.
looks unused – is that correct?
| let configList = XCConfigurationList( | ||
| buildConfigurations: buildConfigurations, | ||
| defaultConfigurationName: defaultConfigurationName, | ||
| defaultConfigurationIsVisible: defaultConfigurationIsVisible | ||
| ) | ||
|
|
||
| return configList |
There was a problem hiding this comment.
nit:
| let configList = XCConfigurationList( | |
| buildConfigurations: buildConfigurations, | |
| defaultConfigurationName: defaultConfigurationName, | |
| defaultConfigurationIsVisible: defaultConfigurationIsVisible | |
| ) | |
| return configList | |
| XCConfigurationList( | |
| buildConfigurations: buildConfigurations, | |
| defaultConfigurationName: defaultConfigurationName, | |
| defaultConfigurationIsVisible: defaultConfigurationIsVisible | |
| ) |
| } | ||
| } | ||
|
|
||
| // |
There was a problem hiding this comment.
I assume this can be removed?
|
|
||
| // MARK: - WorkspaceFixture | ||
|
|
||
| /// Provides references to sample Xcode workspace fixtures used for integration testing. |
There was a problem hiding this comment.
Let's create XcodeProjMapperIntegrationTests
| @Suite | ||
| struct XCWorkspaceMapperTests { | ||
| @Test("Maps workspace without any projects or schemes") | ||
| func testMap_NoProjectsOrSchemes() throws { |
There was a problem hiding this comment.
I'd consider following the // Given // When // Then comments convention that we have in tuist/tuist. It makes tests easier to parse.
| } | ||
|
|
||
| @Test("Resolves other path in XCWorkspaceDataFileRef") | ||
| func testMap_OtherPath() throws { |
There was a problem hiding this comment.
| func testMap_OtherPath() throws { | |
| func testMap_otherPath() throws { |
nit: I would follow this casing convention
| struct TargetDependencyExtensionsTests { | ||
| let sourceDirectory = try! AbsolutePath(validating: "/tmp/TestProject") | ||
|
|
||
| // A dummy global target map for .project dependencies |
There was a problem hiding this comment.
| // A dummy global target map for .project dependencies | |
| // A dummy target map for .project dependencies |
It's not global 🙂
| struct IntegrationTests { | ||
| /// Asserts that the given graph parsed from a workspace fixture matches the provided inline snapshots. | ||
| /// | ||
| /// - Parameters: | ||
| /// - fixture: A closure that returns a `WorkspaceFixture` used to generate the graph. |
There was a problem hiding this comment.
Why are we asserting with snapshots? I'd instead compare the parsed XcodeGraph with the expected XcodeGraph:
#expect(gotXcodeGraph, expectedXcodeGraph)What's the reason we went with asserting on snapshot text which is more unstable than comparing the XcodeGraph directly?
| var schemes = [Scheme]() | ||
| let sharedDataPath = Path(srcPath.pathString) + "xcshareddata/xcschemes" | ||
|
|
||
| if sharedDataPath.exists { | ||
| let schemePaths = try sharedDataPath.children().filter { $0.extension == "xcscheme" } | ||
|
|
||
| // Construct graphType for schemes | ||
| let graphType = GraphType.workspace(workspaceProvider) | ||
| let schemeMapper = XCSchemeMapper() | ||
| for schemePath in schemePaths { | ||
| let xcscheme = try XCScheme(path: schemePath) | ||
| let scheme = try schemeMapper.map(xcscheme, shared: true, graphType: graphType) | ||
| schemes.append(scheme) | ||
| } | ||
| } | ||
|
|
||
| return schemes |
There was a problem hiding this comment.
nit: I generally prefer not using vars if not necessary as it makes it easier to spot where the final values are coming from:
| var schemes = [Scheme]() | |
| let sharedDataPath = Path(srcPath.pathString) + "xcshareddata/xcschemes" | |
| if sharedDataPath.exists { | |
| let schemePaths = try sharedDataPath.children().filter { $0.extension == "xcscheme" } | |
| // Construct graphType for schemes | |
| let graphType = GraphType.workspace(workspaceProvider) | |
| let schemeMapper = XCSchemeMapper() | |
| for schemePath in schemePaths { | |
| let xcscheme = try XCScheme(path: schemePath) | |
| let scheme = try schemeMapper.map(xcscheme, shared: true, graphType: graphType) | |
| schemes.append(scheme) | |
| } | |
| } | |
| return schemes | |
| let sharedDataPath = Path(srcPath.pathString) + "xcshareddata/xcschemes" | |
| if sharedDataPath.exists { | |
| let schemePaths = try sharedDataPath.children().filter { $0.extension == "xcscheme" } | |
| try schemePaths.map { schemePath in | |
| try schemeMapper.map( | |
| try XCScheme(path: schemePath), | |
| shared: true, | |
| graphType: .workspace(workspaceProvider) | |
| ) | |
| } | |
| } else { | |
| return [] | |
| } |
Note I removed schemeMapper initialization as that belongs into the init of XCWorkspaceMapper as it should be dependency injected
|
That was not intended. We should be able to remove now the |
|
Apologies for the delay. I was away due to the holidays. I will respond and update from the feedback within the next few days. I haven't forgotten about it. |
No worries, I figured 🙂 Hope you were able to recharge over the holidays! Looking forward to updates on this, we are still very much excited about this work landing. |
| name: "XcodeProjMapper", | ||
| dependencies: [ | ||
| "XcodeGraph", | ||
| "MetadataProviders", |
There was a problem hiding this comment.
| "MetadataProviders", | |
| "XcodeMetadata", |
I'd keep the convention in this package to use Xcode as a prefix. I feel XcodeMetadata reads a bit better as a module name than MetadataProviders.
There was a problem hiding this comment.
We plan to move away from FatalError: https://community.tuist.dev/t/deprecate-fatalerror/250
Instead, we can leverage the Error's errorDescription instead.
| /// the transformed values will match the original sequence, | ||
| /// except for the values that were transformed into `nil`. | ||
| /// - throws: Rethrows any error thrown by the passed closure. | ||
| public func serialCompactMap<T: Sendable>( |
There was a problem hiding this comment.
I wouldn't make these public from this module. It would be a bit surprising to have these helpers as public in a module called MetadataProviders.
| } | ||
|
|
||
| extension ServiceContext { | ||
| public var logger: Logger? { |
There was a problem hiding this comment.
Would keep this internal as well, so it doesn't unexpectedly spill over to the consumers
There was a problem hiding this comment.
Once this PR is merged, let's not forget about removing these files from tuist/tuist and using this new module instead.
There was a problem hiding this comment.
This should be no longer needed?
| // func createMappedGraph( | ||
| // graphType: XcodeMapperGraphType, | ||
| // projectProviders: [AbsolutePath: MockProjectProvider] | ||
| // ) throws -> XcodeGraph.Graph { | ||
| // let mapper = XcodeGraphMapper() { path in | ||
| // guard let provider = projectProviders[path] else { | ||
| // Issue.record("Unexpected project path requested: \(path)") | ||
| // throw XcodeProjMapper.XcodeProjError.noProjectsFound | ||
| // } | ||
| // return provider | ||
| // } | ||
| // | ||
| // return try mapper.map() | ||
| // } |
|
|
||
| // NOTE: - This is temporary to reduce PR noise | ||
| /// Unzips and returns path to the Fixtures directory, if not already done. | ||
| static func getFixturesPath() throws -> String { |
| ), | ||
| mainGroup: PBXGroup, | ||
| targets: [PBXTarget] = [PBXNativeTarget.test()], | ||
| schemes _: [XCScheme] = [] |
Thanks for another round! 😅 I didn't realize I pushed these commits yet. Of course, any feedback is always appreciated, but I was planning to address or comment on anything unresolved before asking for another look. I’ll tidy things up and give you a shout when it’s fully ready! 🙏 I think most of the moving pieces are in place now. |
Description:
This pull request adds new functionality to convert
.xcworkspaceand.xcodeprojfiles into a high-level graph model (XcodeGraph). It introduces:XcodeProjToGraphModuleA domain-focused module responsible for mapping raw Xcode structures (projects, workspaces, schemes, targets, and dependencies) into a cohesive
XcodeGraphrepresentation. This includes:Mappers:
.xcworkspacecontents into aWorkspacedomain model, identifying referenced.xcodeprojfiles and shared schemes.TargetandSchemedomain entities, capturing essential build, test, and run information.Extensions & Helpers:
Additional utilities to bridge the gap between raw XcodeProj objects and the domain model. For example,
AbsolutePath+Extensionshelps resolve file references, andPlatform+Extensionsassists in correctly inferring the platform from PBXTarget attributes.ProcessRunner & LipoTool:
A utility to run external processes (like
lipo) and integrate their output—such as architecture detection—into the graph model. This allows for richer metadata about binary artifacts and supported architectures.TestSupportModuleThis module centralizes mocks, helpers, and utilities that streamline testing. By providing reusable mock objects (e.g., mock project providers, file structures, and build settings), it reduces boilerplate and fosters consistency across all test suites. Key highlights:
XcodeProjToGraphTestsA comprehensive test suite validating each piece of the mapping logic. The tests are thoughtfully organized by domain:
MapperTests:
ParserTests (ProjectParserTests):
Verifies that the parsing logic for
.xcodeprojand.xcworkspacedirectories is robust and reliable, providing a consistent foundation for the mappers.ProcessTests (LipoToolTests, ProcessRunnerTests):
Checks the integration with external tools (
lipo) and process running capabilities, ensuring errors are handled gracefully and output is captured correctly.Next Steps - Integration Tests:
While this PR focuses on unit-level validation, an upcoming PR will introduce integration tests that run end-to-end scenarios using real
.xcworkspaceand.xcodeprojfixtures. This will confirm that the entire pipeline—from reading disk-based Xcode structures to producing a finalXcodeGraph—functions correctly in practice.While this PR focuses on core functionality, the integration tests validating end-to-end scenarios (e.g., parsing
.xcworkspaceand.xcodeprojintoXcodeGraph) are complete. However, due to the size of the test fixtures (15,000 lines), they were removed to reduce noise.To review and run these tests, check out this commit (before they were removed) : 75bf656. This includes real-world project fixtures and ensures the entire pipeline functions as expected in practical scenarios.
The results for these are browsable here: https://github.com/tuist/XcodeGraph/blob/75bf656adcec3b5fe7f4c034f797961361f9f584/Tests/XcodeProjToGraphTests/IntegrationTests/Projects/IntegrationTests%2BcommandLineToolWithDynamicFramework.swift
Testing & Verification:
swift buildto compile all modules.swift testto execute the full test suite. All tests should pass, confirming correct functionality.Once integrated with CI (e.g., GitHub Actions), these tests will run automatically on each PR or commit, ensuring ongoing stability and reliability.
No Breaking Changes:
This PR adds new modules and tests without altering existing functionality. It forms the foundation for further enhancements and refinements.
Documentation & Maintenance:
Code comments and some documentation are included to guide future developers. With a clear architecture, it will be easier to extend capabilities, add integration tests, and enhance performance or features over time.
Summary:
This PR significantly expands the repository’s capabilities by introducing
XcodeProjToGraphandTestSupportmodules, along with a robust test suite inXcodeProjToGraphTests. Together, they provide a structured path from raw Xcode project/workspace data to a domain-levelXcodeGraphmodel. The next step will be to add integration tests to validate complex real-world scenarios, ensuring the tool’s practicality and reliability for future use cases.