Add support for simple files excludes from sources #5. #135

TheCoordinator · 2017-11-04T16:25:16Z

I understand you may be working on a solution for excludes as hinted in #25, but for now this seems to be resolving the immediate issue of excluding file(s) given a source path.

Please have a look and let me know if you have any feedbacks.

yonaskolb

Nice one @PeymanKh, thanks for this! This is great start to the feature!

yonaskolb · 2017-11-04T20:06:08Z

Sources/ProjectSpec/Source.swift

@@ -43,6 +45,10 @@ extension Source: JSONObjectConvertible {
        let maybeCompilerFlagsArray: [String]? = jsonDictionary.json(atKeyPath: "compilerFlags")
        compilerFlags = maybeCompilerFlagsArray ??
            maybeCompilerFlagsString.map{ $0.split(separator: " ").map{ String($0) } } ?? []
+        let maybeExcludesString: String? = jsonDictionary.json(atKeyPath: "excludes")


We don't need to allow a string version like the compiler flags. Compiler flags were special as they are often defined as a single string in xcode and xcconfig files

yonaskolb · 2017-11-04T20:13:10Z

Sources/XcodeGenKit/PBXProjGenerator.swift

@@ -568,6 +568,7 @@ public class PBXProjGenerator {
            (try path.children().sorted(), path)

        let excludedFiles: [String] = [".DS_Store"]
+        let sourceExcludedFiles: [String] = source.excludes.map { "\(source.path)/\($0)" }


This is a good start. Though ideally I think we'd like to be able to specify excludes via a glob pattern (eg **.DS_Store) PathKit has build in support for glob patterns.
Or at the very least to specify certain file types which could be done with some simple custom logic *.filetype

And would you want to exclude a folder? I don't know, maybe it's not useful?

yonaskolb · 2017-11-04T20:17:40Z

Tests/XcodeGenKitTests/SpecLoadingTests.swift

+                                        Source(path: "sourceWithCompilerFlagsExcludes", compilerFlags: ["-Werror"], excludes: ["Foo.swift"]),
+                                        Source(path: "sourceWithCompilerFlagsExcludesStr", compilerFlags: ["-Werror", "-Wextra"], excludes: ["Foo.swift", "Bar.swift"])]
+
+            try expect(target1.sources) == target1SourcesExpect
            try expect(target2.sources) == [Source(path: "source3")]
        }


It would be great to have some tests in ProjectGeneratorTests under "Sources". You would need to make use of the getFileReference function at the bottom, maybe with a new expectMissingFile (see expectFile for example)

@yonaskolb Thanks for the feedback.

I'm absolutely in for having global patterns and in my opinion excluding the whole directory should also be an option.

On a different note, do you also think it makes sense to have includes as part of Source?

I'm not sure it would make sense to have includes as part of the Source, as that's what the path does. To have multiple sources paths you create multiple sources. But that does raise the question whether excludes should be part of the target and not Source, so that if you have multiple sources you only have to define excludes once. But then again such an approach would probably be used for simply excluding specific file types, and that could be done in the project options level...

Fair enough. Since I'm migrating all the my current work's project files to use Xcodegen instead, felt it can be a good option, but I agree with you, we can get carried away with it.

So i've already implemented pattern matching for excludes for both files and directories, have bumped into an issue with one of the test cases Sources->generates file sources.

Is including files as a source path supposed to work in general?

Here's the test case.

$0.it("generates file sources") { let directories = """ Sources: A: - a.swift - B: - b.swift - c.jpg """ try createDirectories(directories) target.sources = [ "Sources/A/a.swift", "Sources/A/B/b.swift", "Sources/A/B/c.jpg", ] spec.targets = [target] let project = try getPbxProj(spec) try project.expectFile(paths: ["Sources/A", "a.swift"], names: ["A", "a.swift"], buildPhase: .sources) try project.expectFile(paths: ["Sources/A/B", "b.swift"], names: ["B", "b.swift"], buildPhase: .sources) try project.expectFile(paths: ["Sources/A/B", "c.jpg"], names: ["B", "c.jpg"], buildPhase: .resources) }

When this gets resolved I'll push so that you can have a look at the changes.

Yeah adding file sources was added recently via #106

yonaskolb · 2017-11-04T20:30:34Z

Sources/XcodeGenKit/PBXProjGenerator.swift

@@ -568,6 +568,7 @@ public class PBXProjGenerator {
            (try path.children().sorted(), path)

        let excludedFiles: [String] = [".DS_Store"]


Just a note, these default excludedFiles should be moved up into the spec options at some point.

TheCoordinator · 2017-11-06T10:41:56Z

@yonaskolb There you go, updated with pattern matching and tests.

yonaskolb · 2017-11-06T14:46:10Z

Sources/ProjectSpec/Source.swift

        let maybeExcludesArray: [String]? = jsonDictionary.json(atKeyPath: "excludes")
-        excludes = maybeExcludesArray ??
-            maybeExcludesString.map{ $0.split(separator: " ").map{ String($0) } } ?? []        
+        excludes = maybeExcludesArray ?? []


You can just replace this with excludes = jsonDictionary.json(atKeyPath: "excludes") ?? []

yonaskolb

Looking good! I left some comments

yonaskolb · 2017-11-06T16:56:37Z

Sources/XcodeGenKit/PBXProjGenerator.swift

@@ -647,4 +657,52 @@ public class PBXProjGenerator {
        groups.insert(group, at: 0)
        return (allSourceFiles, groups)
    }
+
+    func getSourceChildren(sourceMetadata source: Source, dirPath: Path) throws -> [Path] {
+        let excludedFiles = [".DS_Store"].map { dirPath + Path($0) }


Can we call this defaultExcludedFiles

yonaskolb · 2017-11-06T16:57:42Z

Sources/XcodeGenKit/PBXProjGenerator.swift

+        }
+    }
+
+    func getSourceExcludes(sourceMetadata source: Source, dirPath: Path) -> [Path] {


Can we make this func live within getSourceChildren as a nested function, as it's not used anywhere else. There are so many top level source functions now that I want to cut down on them

We could also just path the excludes here. It doesn't need to know about the whole Source

yonaskolb · 2017-11-06T16:59:52Z

Sources/XcodeGenKit/PBXProjGenerator.swift

+                    return [$0]
+                }
+
+                return Path.glob("\($0.string)*/**") + Path.glob("\($0.string)**")


Could you explain what this is doing? Is this finding all files within the directory? If so you can use $0.recursiveChildren

Yes, to exclude all files and directories recursively.

OK I'll try that.

yonaskolb · 2017-11-06T17:07:50Z

Tests/XcodeGenKitTests/ProjectGeneratorTests.swift

+                let excludes = [
+                    "B",
+                    "*.h",
+                    "f.swift"


If we were using true glob patterns, f.swift wouldn't be removed, as it's in sub directory. Same goes for .h files that are in sub directories. That might be ok though, as this format is easier? What do you think?

Is it currently possible to exclude certain files from a sub directory as we're not using the exact glob spec? If so can we add a case eg. J/*.h

So the glob patterns we are using are recursive. That is probably desirable for the ease of use as you said. For example you'd like to exclude a certain type within the whole source.

Having said that and thinking about it a bit more, I think, excluding files recursive should be provided as an option and keep it off by default. Realistically it's probably best to keep it non-recursive for now, and provide a recursive option in the future if there is demand for it.

It is also possible to exclude file(s) from sub directory with a pattern. Will add the test case.

What do you think of this? I have removed recursive patterns based on the reasoning above.

Any other cases in mind?

$0.it("generates source groups with excludes") { let directories = """ Sources: A: - a.swift - B: - b.swift B: - b.swift C: - c.swift - c.m - c.h D: - d.h - d.m E: - e.jpg - e.h - e.m - F: - f.swift G: H: - h.swift """ try createDirectories(directories) let excludes = [ "B", "C/*.h", "d.m", "E/F/*.swift", "G/H/" ] target.sources = [Source(path: "Sources", excludes: excludes)] spec.targets = [target] let project = try getPbxProj(spec) try project.expectFile(paths: ["Sources", "A", "a.swift"], buildPhase: .sources) try project.expectFile(paths: ["Sources", "C", "c.swift"], buildPhase: .sources) try project.expectFile(paths: ["Sources", "C", "c.m"], buildPhase: .sources) try project.expectFile(paths: ["Sources", "D", "d.h"]) try project.expectFile(paths: ["Sources", "D", "d.m"], buildPhase: .sources) try project.expectFile(paths: ["Sources", "E", "e.jpg"], buildPhase: .resources) try project.expectFile(paths: ["Sources", "E", "e.m"], buildPhase: .sources) try project.expectFile(paths: ["Sources", "E", "e.h"]) try project.expectFileMissing(paths: ["Sources/B", "b.swift"]) try project.expectFileMissing(paths: ["Sources/C", "c.h"]) try project.expectFileMissing(paths: ["Sources/E/F", "f.swift"]) try project.expectFileMissing(paths: ["Sources/G/H", "h.swift"]) }

I would like to have support in for proper recursive glob patterns.
Something like **/*.a which would remove all .h files. Is that possible with how you've implemented it? If so can we add a test case for it

I think it should be possible. Will add it tomorrow :)

yonaskolb · 2017-11-06T17:09:40Z

Sources/XcodeGenKit/PBXProjGenerator.swift

+                + excludedFiles)
+
+        return try dirPath.children().sorted()
+        .filter {


If we sort after we filter it's technically faster. Probably pre-mature optimization, but maybe someone has thousands of files

yonaskolb · 2017-11-06T17:10:50Z

Sources/XcodeGenKit/PBXProjGenerator.swift

@@ -562,20 +562,20 @@ public class PBXProjGenerator {
    }

    func getSources(sourceMetadata source: Source, path: Path, depth: Int = 0) throws -> (sourceFiles: [SourceFile], groups: [PBXGroup]) {
-        // if we have a file, move it to children and use the parent as the path
        let (children, path) = path.isFile ?
            ([path], path.parent()) :


I guess no-one would specify a file path and then exclude it. So not taking care of the file case is fine here 👍

yonaskolb · 2017-11-06T17:12:27Z

Sources/XcodeGenKit/PBXProjGenerator.swift

            allSourceFiles += subGroups.sourceFiles
-            groupChildren.append(subGroups.groups.first!.reference)


It's good this is in there now, as this would have been a problem with empty directories 👍

yonaskolb · 2017-11-06T17:13:51Z

Tests/XcodeGenKitTests/ProjectGeneratorTests.swift

+                try createDirectories(directories)
+
+                let excludes = [
+                    "B",


Can we try excluding a sub directory? eg G/H

yonaskolb · 2017-11-06T17:17:20Z

Tests/XcodeGenKitTests/ProjectGeneratorTests.swift

+    func expectFileMissing(paths: [String], names: [String]? = nil) throws {
+        let names = names ?? paths
+        if getFileReference(paths: paths, names: names) != nil {
+            throw failure("Found file at path \(paths.joined(separator: "/").quoted) and name \(paths.joined(separator: "/").quoted)")


Can you change to Found unexpected file ...

yonaskolb · 2017-11-16T10:33:21Z

@PeymanKh I don't think the excludes are working properly.

The tests that validate this are actually incorrect, so they are passing when they shouldn't

try project.expectFileMissing(paths: ["Sources/B", "b.swift"])

should be

try project.expectFileMissing(paths: ["Sources", "B", "b.swift"])

TheCoordinator · 2017-11-16T13:01:03Z

@yonaskolb I see, saw variations of expectFile being used, I'll check whether this has to do with the test expectFileMissing or the actual implementation.

Thanks for the heads up.

yonaskolb · 2017-11-16T14:08:05Z

Cool. I’ve left it out of the change logs and docs for now

TheCoordinator · 2017-11-19T13:10:33Z

@yonaskolb I've fixed the issue, would you like me to open a new branch for this and send new pull request or should I update this branch?

yonaskolb · 2017-11-19T13:29:21Z

Awesome. Please open a new PR

Add support for simple files excludes from sources yonaskolb#5.

0da3b7b

yonaskolb reviewed Nov 4, 2017

View reviewed changes

TheCoordinator added 2 commits November 5, 2017 20:54

Merge branch 'upstream-master' into excludes

3645d99

Add pattern matching for Source excludes yonaskolb#5.

0da78a0

yonaskolb reviewed Nov 6, 2017

View reviewed changes

Simplify getting Source.excludes from jsonDictionary.

7a6476f

yonaskolb reviewed Nov 6, 2017

View reviewed changes

TheCoordinator and others added 3 commits November 7, 2017 15:06

Remove recursive pattern matching and formatting.

7e6a45e

Merge branch 'master' into excludes

7cf7ee6

Merge branch 'master' into excludes

11f7480

yonaskolb merged commit 91729a9 into yonaskolb:master Nov 10, 2017

yonaskolb mentioned this pull request Nov 12, 2017

Target source excludes #5

Closed

		@@ -568,6 +568,7 @@ public class PBXProjGenerator {
		(try path.children().sorted(), path)

		let excludedFiles: [String] = [".DS_Store"]

		allSourceFiles += subGroups.sourceFiles
		groupChildren.append(subGroups.groups.first!.reference)

Add support for simple files excludes from sources #5. #135

Add support for simple files excludes from sources #5. #135

Conversation

TheCoordinator commented Nov 4, 2017

yonaskolb left a 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

TheCoordinator commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonaskolb left a 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

TheCoordinator Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheCoordinator Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

yonaskolb Nov 7, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonaskolb commented Nov 16, 2017

TheCoordinator commented Nov 16, 2017

yonaskolb commented Nov 16, 2017

TheCoordinator commented Nov 19, 2017

yonaskolb commented Nov 19, 2017

TheCoordinator Nov 7, 2017 •

edited

Loading

TheCoordinator Nov 7, 2017 •

edited

Loading

yonaskolb Nov 7, 2017 •

edited

Loading