Add support for Copy Files build phase #345

brentleyjones · 2018-07-17T20:38:01Z

Related to #344, this allows manually setting up Copy Files build phases.

brentleyjones · 2018-07-17T21:15:21Z

Sources/ProjectSpec/TargetSource.swift

 case .none: return nil
 }
 }
 }
+
+ public struct CopyFiles: Equatable, Hashable {


Thinking about this, maybe this should be called CopyFilesBuildPhaseSettings. I know it's longer, but I think it makes it clear what it's for.

brentleyjones · 2018-07-17T21:23:03Z

Docs/ProjectSpec.md

 - `none` - Will not be added to any build phases
+- [ ] **copyFiles**: **[Copy Files](#copy-files)** - Options for the Copy Files build phase. If specified the `buildPhase` needs to be `copyFiles` or unspecified (in which case it will default to `copyFiles`).


Instead of this I could make it so when you specify copyFiles for buildPhase you have to do it as an object:

- buildPhase: type: copyFiles destination: productsDirectory subpath: include/$(PRODUCT_NAME)

This might complicate it too much though, which is why I went the route I did.

Also, I believe the way people will specify this is just by using the copyFiles object and not specifying a buildPhase manually.

brentleyjones · 2018-07-17T21:25:30Z

Sources/ProjectSpec/SpecValidation.swift

@@ -74,6 +74,10 @@ extension Project {
 if !source.optional && !sourcePath.exists {
 errors.append(.invalidTargetSource(target: target.name, source: sourcePath.string))
 }
+
+ if let buildPhase = source.buildPhase, buildPhase != .copyFiles, source.copyFiles != nil {


I need this to also pick up the case when build phase is copyFiles but a copyFiles section doesn't exist.

brentleyjones · 2018-07-17T21:44:24Z

Sources/ProjectSpec/TargetSource.swift

@@ -12,6 +12,7 @@ public struct TargetSource: Equatable {
 public var type: SourceType?
 public var optional: Bool
 public var buildPhase: BuildPhase?
+ public var copyFiles: CopyFiles?


Instead of adding another field, I’m going to make BuildPhase have an associated value. It makes my follow up PR easier as well as cleaning up the paths that don’t care about copyFiles.

toshi0383

Great!

brentleyjones · 2018-07-19T22:29:27Z

I'll update CHANGELOG.md soon. I keep forgetting about that step.

brentleyjones · 2018-07-20T18:51:04Z

@yonaskolb Not sure on the protocol here. Is this fine for me to merge?

yonaskolb · 2018-07-21T01:36:36Z

There were just a couple of comments I wanted to make. Just on the road at the moment, but will get to them all tomorrow 😀👍

yonaskolb · 2018-07-18T11:39:39Z

Sources/XcodeGenKit/PBXProjGenerator.swift

+ dstSubfolderSpec: copyFiles.destination.destination,
+ files: buildPhaseFiles
+ )
+ let copyFilesBuildPhase = createObject(id: target.name, buildPhase)


Can the id include string copy files similar to the other PBXCopyFilesBuildPhases. That makes the resulting UUID handle clashes for when there is more than one PBXCopyFilesBuildPhase

Yes, I’ll adjust 🙂.

yonaskolb · 2018-07-22T02:33:15Z

Sources/ProjectSpec/TargetSource.swift

 case none
+ // Not currently exposed as selectable options, but here to translate to PBXCopyFilesBuildPhase completely
+ case frameworks


Do we need these in here?

Yes, since this type is now used where xcproj ones were before, to allow defaulting to work correctly (see the modulemap PR for an example).

yonaskolb · 2018-07-22T02:37:29Z

Sources/ProjectSpec/TargetSource.swift


 public var buildPhase: xcproj.BuildPhase? {
 switch self {
 case .sources: return .sources
 case .headers: return .headers
 case .resources: return .resources
+ case .copyFiles(_): return .copyFiles


nitpick: we don't need the (_). This would probably get removed by make format_code anyway, so not really an issue

I did it without and got an error. I’ll try again.

yonaskolb · 2018-07-22T02:42:26Z

Sources/XcodeGenKit/PBXProjGenerator.swift

+ guard case let .copyFiles(copyFilesSettings)? = sourceFile.buildPhase else { continue }
+ var sourceFilesForCopyFiles = sourceFilesByCopyFiles[copyFilesSettings] ?? []
+ sourceFilesForCopyFiles.append(sourceFile)
+ sourceFilesByCopyFiles[copyFilesSettings] = sourceFilesForCopyFiles


You can use Swift 4's new dictionary accessors:

sourceFilesByCopyFiles[copyFilesSettings, default: []].append(sourceFile)

yonaskolb · 2018-07-22T05:07:05Z

Sources/ProjectSpec/TargetSource.swift

+ if let string = string {
+ chosenString = string
+ } else if copyFilesSettings != nil {
+ chosenString = "copyFiles"


The downside of this is that if they have a TargetSource with mixed files, there's no way to just specify the settings for the ones that will be in copyFiles by default, because if you do that everything will be moved to the copyFiles build phase.
I would be ok with keeping the copyFiles struct out of the enum like you had it before. The validation error could then be thrown in the generator when searching through the files and finding any copyFiles build phase SourceFiles (like modulemap) and it not having a copyFiles settings.

Are there other sorts of files you would want to add to copyFiles, or is it just modulemap?

Header files in the case of static libraries.

Let me think about your downside comment. When I had it different before it would default the whole sources to the copy files build phase, and the way I see you recommending it is that if it were determined to need to be copied it should use these settings. Right now I’m treating it exactly like all the other build phases and if specified they override any default logic. This phase just happens to require additional configuration options.

yonaskolb · 2018-07-22T05:07:37Z

Docs/ProjectSpec.md

@@ -279,10 +281,31 @@ targets:
 - "-Wextra"
 - path: MyOtherTargetSource3
 compilerFlags: "-Werror -Wextra"
+ - path: ModuleMaps
+ copyFiles:


indentation

brentleyjones · 2018-07-23T13:41:44Z

@yonaskolb I adressed all your feedback, expect for
#345 (comment). With that I changed the structure so copyFiles is simply an object specified for buildPhase. It's only special in the sense that it has additional required fields to be specified. It behaves the same as all the other build phases, if you specify it you mean it for all of the files (so use excludes or be specific). Defaults will be handled by #346 and a PR I plan on making soon for static library headers.

yonaskolb · 2018-07-23T14:02:58Z

Cool, I'll take a look later 😄👍
In the future if a PR has been reviewed could you add new commits instead of rebasing? It makes it easier to see the changes since reviewing. We always have the option of squashing on merge if there are lots of commits

brentleyjones · 2018-07-23T14:04:13Z

Will do! I'll add commits next time (I rebased a couple other PRs, sorry, I won't in the future).

yonaskolb · 2018-07-23T14:06:53Z

Also I should clarify. It's ok to rebase off the target branch to get new changes, but I meant not amending or squashing new commits 👍

brentleyjones · 2018-07-24T15:07:58Z

@yonaskolb Rebased and ready to go.

yonaskolb · 2018-07-24T12:18:28Z

Docs/ProjectSpec.md

+ - `sharedSupport`
+ - `plugins`
+ - [x] **subpath**: **String** - The path inside of the destination to copy the files.
+- [x] **subpath**: **String** - The path inside of the destination to copy the files.


duplicate line

yonaskolb · 2018-07-29T11:28:29Z

Sources/ProjectSpec/TargetSource.swift

 case none
+ // Not currently exposed as selectable options, but used internally
+ case frameworks


@yonaskolb I’m not sure what you are referring to here. Do you want a newline before the comment, the comment to be indented differently, or something else?

Hmm, it's not showing up now. I swear case frameworks looked indented differently before

yonaskolb · 2018-07-29T11:36:06Z

Sources/XcodeGenKit/PBXProjGenerator.swift

+ let copyFilesBuildPhase = createObject(
+ id: "copy files" + copyFiles.destination.rawValue + copyFiles.subpath + target.name,
+ PBXCopyFilesBuildPhase(
+ dstPath: copyFiles.subpath,


I guess you've tested if a the default subpath of "" is valid, as opposed to nil? As the value is optional in PBXCopyFilesBuildPhase. Just trying to minimise diffs when projects are opened in Xcode

I’ll reaffirm.

The default is "". If you don't specify it, it won't exist, which is still valid, but to match what Xcode does naturally it's "" specified. Xcode won't change when opening either way.

👍
Sometimes Xcode won't change the project on open, but if something else changes (like reordering some files is an easy way to test), then it will rewrite things in it's preferred way

yonaskolb · 2018-07-29T11:42:05Z

Sources/ProjectSpec/TargetSource.swift

+
+ public init(jsonDictionary: JSONDictionary) throws {
+ destination = try jsonDictionary.json(atKeyPath: "destination")
+ subpath = jsonDictionary.json(atKeyPath: "subpath") ?? ""


Would the default be nil?

I'll adjust the docs to say it's optional. We should still default it to "" though to match Xcode behavior (there isn't a way to make it not "" by using just Xcode).

yonaskolb · 2018-07-29T11:44:50Z

Sources/ProjectSpec/SpecParsingError.swift

@@ -5,6 +5,7 @@ public enum SpecParsingError: Error, CustomStringConvertible {
 case unknownTargetPlatform(String)
 case invalidDependency([String: Any])
 case unknownSourceBuildPhase(String)
+ case invalidSourceCopyFilesPhase


This might get unwieldy if we add more build phase enum options. Can we just used the existing unknownSourceBuildPhase but renamed to invalidSourceBuildPhase along with the error. Then the string can be copyFiles must be a dictionary

Yep. I’ll adjust.

Consolidate `.unknownSourceBuildPhase` and `.invalidSourceCopyFilesPhase` into one.

brentleyjones · 2018-07-30T13:47:33Z

@yonaskolb All feedback addressed besides for the indentation one (I left a question). This is G2G 😄.

yonaskolb · 2018-07-31T12:28:22Z

Docs/ProjectSpec.md

@@ -260,21 +260,33 @@ A source can be provided via a string (the path) or an object of the form:
 - `sources` - Compile Sources phase
 - `resources` - Copy Bundle Resources phase
 - `headers` - Headers Phase
- - `none` - Will not be added to any build phases


Has none been removed?

No. I’m not sure how that happened. I’ll fix it.

yonaskolb · 2018-07-31T12:30:58Z

Sources/ProjectSpec/TargetSource.swift

+ case "headers": self = .headers
+ case "resources": self = .resources
+ case "copyFiles":
+ throw SpecParsingError.invalidSourceBuildPhase("copyFiles must specify directory")


Should this be copyFiles must specify a \"destination\" and optional \"subpath\"

Yes, that's more descriptive.

yonaskolb · 2018-07-31T12:32:03Z

Sources/ProjectSpec/TargetSource.swift

+ throw SpecParsingError.invalidSourceBuildPhase("copyFiles must specify directory")
+ case "none": self = .none
+ default:
+ throw SpecParsingError.invalidSourceBuildPhase("Unknown build phase \(string.quoted)")


I guess this can just be the string

yonaskolb · 2018-07-31T13:01:27Z

Feel free to merge

yonaskolb · 2018-07-31T13:09:44Z

Nice work @brentleyjones! 🎉

brentleyjones added the enhancement label Jul 17, 2018

brentleyjones requested a review from yonaskolb July 17, 2018 20:38

brentleyjones force-pushed the modulemap branch from a8dd709 to fabff55 Compare July 17, 2018 20:54

brentleyjones commented Jul 17, 2018

View reviewed changes

brentleyjones force-pushed the modulemap branch from fabff55 to 6d17948 Compare July 17, 2018 23:17

brentleyjones mentioned this pull request Jul 17, 2018

Automatically copy modulemap files into products directory #346

Merged

toshi0383 approved these changes Jul 19, 2018

View reviewed changes

brentleyjones mentioned this pull request Jul 19, 2018

Better static library support #350

Closed

brentleyjones force-pushed the modulemap branch from 6d17948 to 7c31800 Compare July 20, 2018 14:09

yonaskolb reviewed Jul 22, 2018

View reviewed changes

brentleyjones force-pushed the modulemap branch 2 times, most recently from 68b1cb4 to 0bb4dd5 Compare July 23, 2018 13:24

brentleyjones mentioned this pull request Jul 23, 2018

macOS: XPC Service dependency copied to resource folder rather embedded. #334

Closed

brentleyjones force-pushed the modulemap branch from 0bb4dd5 to afc93ee Compare July 24, 2018 15:07

brentleyjones mentioned this pull request Jul 25, 2018

Add names to non-generic Copy Files phases #360

Merged

yonaskolb reviewed Jul 29, 2018

View reviewed changes

brentleyjones added 2 commits July 30, 2018 08:14

Add support for Copy Files build phase

5e96eed

Refactor SpecParsingError.unknownSourceBuildPhase

d1c3ac1

Consolidate `.unknownSourceBuildPhase` and `.invalidSourceCopyFilesPhase` into one.

brentleyjones force-pushed the modulemap branch from afc93ee to d1c3ac1 Compare July 30, 2018 13:15

yonaskolb mentioned this pull request Jul 31, 2018

Automatically copy public headers for static libraries #365

Merged

yonaskolb reviewed Jul 31, 2018

View reviewed changes

brentleyjones added 2 commits July 31, 2018 07:33

Add back none build phase to ProjectSpec.md

0090d75

Cleanup SpecParsingError.invalidSourceBuildPhase

b44dc46

yonaskolb approved these changes Jul 31, 2018

View reviewed changes

brentleyjones merged commit 717c899 into yonaskolb:master Jul 31, 2018

brentleyjones deleted the modulemap branch July 31, 2018 13:02

		- `none` - Will not be added to any build phases
		- [ ] copyFiles: [Copy Files](#copy-files) - Options for the Copy Files build phase. If specified the `buildPhase` needs to be `copyFiles` or unspecified (in which case it will default to `copyFiles`).

Add support for Copy Files build phase #345

Add support for Copy Files build phase #345

Conversation

brentleyjones commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toshi0383 left a comment

Choose a reason for hiding this comment

brentleyjones commented Jul 19, 2018

brentleyjones commented Jul 20, 2018

yonaskolb commented Jul 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentleyjones commented Jul 23, 2018

yonaskolb commented Jul 23, 2018

brentleyjones commented Jul 23, 2018

yonaskolb commented Jul 23, 2018

brentleyjones commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentleyjones commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonaskolb Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonaskolb commented Jul 31, 2018

yonaskolb commented Jul 31, 2018

yonaskolb Jul 31, 2018 •

edited

Loading