Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support External Target Reference #655

Merged
merged 43 commits into from
Oct 27, 2019

Conversation

kateinoigakukun
Copy link
Collaborator

@kateinoigakukun kateinoigakukun commented Sep 14, 2019

Motivation

XcodeGen supports generating schemes but it only allows to reference it's own targets in project.yml.
We often want to execute unit tests over xcodeprojs at once.

Changed

This pr added externalProject property which enables to build other project targets.

And added new syntax to reference other project target.

projectReferences:
  FooLib:
    path: path/to/FooLib.xcodeproj
schemes:
  TestTarget:
    build:
      targets:
        FooLib/Target7: ["run"]

@yonaskolb
Copy link
Owner

I'd like to move towards being able to reference other projects in more places in an easy way. This might be a good place to start to doing that. I think target: String could be turned into target: TargetReference

struct TargetReference {
  let name: String
  enum Location {
  	case local // better name? spec?
  	case project(path: String)
  }

  init(string: String) {
	..
  }	
}

(In the future this could include a case for named projects when the spec supports multiple projects.) We just need some syntax to describe it in a string so it can be defined and parsed. Something like MyProject.xcodeproj/target

@kateinoigakukun
Copy link
Collaborator Author

kateinoigakukun commented Sep 22, 2019

@yonaskolb I implemented ProjectName/TargetName syntax to represent other project target.

What do you think about this syntax?

externalProjects:
  FooLib:
    path: Libraries/FooLib.xcodeproj
schemes:
  TestTarget:
    build:
      targets:
        FooLib/Target7: ["run"]

@kateinoigakukun
Copy link
Collaborator Author

@yonaskolb Sorry for pinging you, cloud you review again?

Docs/ProjectSpec.md Outdated Show resolved Hide resolved
Sources/ProjectSpec/Scheme.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/Scheme.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/TargetScheme.swift Outdated Show resolved Hide resolved
Tests/XcodeGenKitTests/ProjectSpecTests.swift Outdated Show resolved Hide resolved
Tests/XcodeGenKitTests/SchemeGeneratorTests.swift Outdated Show resolved Hide resolved
Tests/XcodeGenKitTests/SpecLoadingTests.swift Outdated Show resolved Hide resolved
Tests/Fixtures/scheme_test/test_project.yml Show resolved Hide resolved
Copy link
Collaborator

@giginet giginet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent features! We can't wait to use them.

I noted trivial points.

Sources/ProjectSpec/ProjectReference.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/ProjectReference.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/ProjectReference.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/TargetReference.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/TargetReference.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/TargetReference.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/TargetReference.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/TargetReference.swift Outdated Show resolved Hide resolved
projectFilePath = projectReference.path
case .local:
pbxProj = self.pbxProj
projectFilePath = "\(self.project.name).xcodeproj"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true?

Can you try project.basepath to get xcodeproj path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, ReferencedContainer is relative to a directory which has xcodeproj

Sources/XcodeGenKit/SchemeGenerator.swift Show resolved Hide resolved
guard let _buildableName =
project.getTarget(buildTarget.target.name)?.filename ??
project.getAggregateTarget(buildTarget.target.name)?.name else {
throw SchemeGenerationError.unresolvableBuildableName(buildTarget)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While there's nothing bad with having specific errors thrown here, checks like this occur in the SpecValidation phase. Adding a new set of SchemeGenerationErrors just introduces more maintenance. The rest of the codebase gracefully fails at points like this. Thoughts @kateinoigakukun, @giginet?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right this condition will be not caused by user inputs, seems to be caused by an implementation error.
Other exceptions will be occurred by user inputs, however, here is not.
It's better to call fatalError instead of throwing the exceptions here.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @kateinoigakukun, could you revert to how you had it before then

Sources/ProjectSpec/SpecValidation.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/Scheme.swift Outdated Show resolved Hide resolved
Sources/ProjectSpec/TargetReference.swift Outdated Show resolved Hide resolved
Sources/XcodeGenKit/SchemeGenerator.swift Outdated Show resolved Hide resolved
Sources/XcodeGenKit/SchemeGenerator.swift Outdated Show resolved Hide resolved
guard let projectReference = self.project.getProjectReference(project) else {
throw SchemeGenerationError.missingProject(project)
}
pbxProj = try XcodeProj(pathString: projectReference.path).pbxproj
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could become a performance issue, especially if there are lots of target references. We could at least cache the projects in this class

var projects: [String: XcodeProj]

As far as I understand you're only loading these projects to get the proper file extension for the target right? Does the buildable name require that? Cause if it doesn't we can just do away with loading the projects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could at least cache the projects in this class

Right, I added cache logic.

As far as I understand you're only loading these projects to get the proper file extension for the target right? Does the buildable name require that? Cause if it doesn't we can just do away with loading the projects.

PBXProj is also used to get blueprint of target https://github.com/yonaskolb/XcodeGen/pull/655/files#diff-b0109f56886f826d76bacb6b15dbf86dR132

Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, nice work @kateinoigakukun!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants