SR-1741: Add non-source files from source directories and root, ignore files from .gitignore #1507

ViktorSimko · 2018-02-23T16:29:26Z

No description provided.

aciidgh

Thanks for working on this! I left some comments inline.

aciidgh · 2018-02-26T21:27:01Z

Sources/Xcodeproj/generate().swift

+    }
+
+    // Get the ignored files and exclude them
+    let ignoredFiles = try repositoryProvider.openCheckout(at: srcroot).getIgnoredFiles()


I think we need to handle the case when there is no repository at srcroot.

aciidgh · 2018-02-26T21:31:02Z

Sources/Xcodeproj/generate().swift

+        if !isFile($0) { return false }
+        if let `extension` = $0.extension {
+            if SupportedLanguageExtension.cFamilyExtensions.contains(`extension`) { return false }
+            if SupportedLanguageExtension.swiftExtensions.contains(`extension`) { return false }


We can use validExtensions here instead.

aciidgh · 2018-02-26T21:32:24Z

Sources/Xcodeproj/generate().swift

@@ -181,3 +199,17 @@ func findDirectoryReferences(path: AbsolutePath) throws -> [AbsolutePath] {
        return isDirectory($0)
    })
 }
+
+/// Finds the non-source files from `path` recursively


Since recursively is a parameter, this should be reworded.

aciidgh · 2018-02-26T21:32:55Z

Sources/Xcodeproj/generate().swift

+    // Find non-source files in the source directories and root that should be added
+    // as a reference to the project.
+    var extraFiles = try findNonSourceFiles(path: srcroot)
+    if let sourcesExtraFiles = try? findNonSourceFiles(path: srcroot.appending(component: "Sources"), recursively: true) {


I think we don't need to use try? here.

aciidgh · 2018-02-26T21:34:41Z

Sources/Xcodeproj/generate().swift

+
+    // Get the ignored files and exclude them
+    let ignoredFiles = try repositoryProvider.openCheckout(at: srcroot).getIgnoredFiles()
+    extraFiles = extraFiles.filter {


This could just be extraFiles = extraFiles.filter(ignoredFiles.contains).

aciidgh · 2018-02-26T21:38:05Z

Sources/SourceControl/GitRepository.swift

@@ -391,6 +391,16 @@ public class GitRepository: Repository, WorkingCheckout {
        return localFileSystem.isDirectory(AbsolutePath(firstLine))
    }

+    public func getIgnoredFiles() -> [AbsolutePath] {


This could have a big impact on performance because we will end up looking at each file in the .build directory, which is generally big. We should find a way to do this in a better way.

Thanks for the review.
Yeah, this slipped my mind.
Maybe we could use git check-ignore on the files found by findNonSourceFiles.
Or should we also ignore the files if there isn't a repository for the package?

Using git check-ignore sounds like a good idea. And if there isn't a repository for the package, then by definition no files are ignored in a Git sense, so I don't think there's an issue there.

ah I meant to say that we should make sure we don't throw when there is no repository.

aciidgh · 2018-03-01T21:24:02Z

Sources/Xcodeproj/pbxproj().swift

@@ -294,7 +297,6 @@ func xcodeProject(
            let name = (sourcesGroup == nil ? groupName : target.name)
            let group = (sourcesGroup ?? parentGroup)
                .addGroup(path: (path == "." ? "" : path), pathBase: .projectDir, name: name)
-


Please avoid whitespace diffs.

aciidgh · 2018-03-01T21:28:04Z

Sources/Xcodeproj/generate().swift

+    let ignoredFiles = try repositoryProvider.openCheckout(at: srcroot).getIgnoredFiles()
+    extraFiles = extraFiles.filter {
+        return !ignoredFiles.contains($0)
+    }


I think we can pass a closure to walk function that will avoid iterating the ignored directories, which is a little better for performance. We should also get some tests to prove this PR works.

aciidgh · 2018-03-01T21:28:55Z

Sources/SourceControl/GitRepository.swift

+                args: Git.tool, "-C", path.asString, "ls-files", "-o", "-i", "--exclude-standard").chomp()
+            return result?.split(separator: "\n").map(String.init).map {
+                return path.appending(RelativePath($0))
+            } ?? []


We should not ignore the failures here and mark this method throwing.

giginet · 2018-03-04T16:48:26Z

How should we handle global .gitignore?

hartbit · 2018-06-12T21:23:38Z

@giginet What's the particular problem with a global .gitignore? Doesn't check-ignore already handle them?

ViktorSimko · 2018-06-14T14:25:11Z

Yes, check-ignore handles every input file to the exclusion.

ViktorSimko · 2018-06-14T21:38:03Z

I've added tests for this.

ViktorSimko · 2018-06-19T12:12:22Z

@aciidb0mb3r Could you please review this?

hartbit · 2018-06-19T18:59:39Z

Sources/Xcodeproj/generate().swift

+    if try repositoryProvider.checkoutExists(at: srcroot) {
+        // Get the ignored files and exclude them
+        let repository = try repositoryProvider.openCheckout(at: srcroot)
+        extraFiles = try extraFiles.filter { try !repository.isIgnored($0) }


Minor nitpick: Can you surround the closure with parentheses to match the informal project style? For reference, it follows the Rule of Kevin (“When a trailing closure argument is functional, use parentheses. When it is procedural, use braces.”)

hartbit · 2018-06-19T19:00:01Z

Minor stylistic comment, but this LGTM! @aciidb0mb3r what do you think?

ViktorSimko · 2018-06-19T21:24:55Z

@hartbit Thanks for the review! I've fixed the parentheses around the closures.

aciidgh · 2018-06-20T16:29:15Z

Sources/SourceControl/GitRepository.swift

@@ -92,6 +92,13 @@ public class GitRepositoryProvider: RepositoryProvider {
        }
    }


We should land git related changes in a separate PR.

aciidgh · 2018-06-20T16:34:49Z

Sources/SourceControl/GitRepository.swift

@@ -387,6 +397,22 @@ public class GitRepository: Repository, WorkingCheckout {
        return localFileSystem.isDirectory(AbsolutePath(firstLine))
    }

+    /// Returns true if the file at `path` is ignored by `git`
+    public func isIgnored(_ path: AbsolutePath) throws -> Bool {


How about isGitIgnored?

aciidgh · 2018-06-20T16:37:14Z

Sources/Xcodeproj/generate().swift

+    // as a reference to the project.
+    var extraFiles = try findNonSourceFiles(path: srcroot)
+
+    let sourcesDirectory = srcroot.appending(component: "Sources")


We should use the (root) package object to get this path as it maybe something else other than Sources.

aciidgh · 2018-06-20T16:38:15Z

Sources/Xcodeproj/generate().swift

+    if try repositoryProvider.checkoutExists(at: srcroot) {
+        // Get the ignored files and exclude them
+        let repository = try repositoryProvider.openCheckout(at: srcroot)
+        extraFiles = try extraFiles.filter({ try !repository.isIgnored($0) })


This is going to shell out for every single extra file which will have a huge performance impact. Is there a way to pass multiple files in an invocation?

aciidgh · 2018-06-20T16:38:44Z

Sources/Xcodeproj/generate().swift

@@ -77,10 +79,32 @@ public func generate(
    // references in the project.
    let extraDirs = try findDirectoryReferences(path: srcroot)

+    // Find non-source files in the source directories and root that should be added


We should factor out this entire diff into a new method.

ViktorSimko · 2018-06-20T22:04:51Z

@aciidb0mb3r Thanks for the review, I've made the requested changes.

hartbit · 2018-06-20T23:08:58Z

@aciidb0mb3r I don’t agree with calling the function isGitIgnored: it’s a requirement on Repository and won’t make any sense if and when we provide an implementation for any other repository system (SVN, Mercurial). I think it should definitely be called isIgnored.

aciidgh · 2018-06-20T23:38:46Z

Ah, thats a good point. Maybe we should call it something else? isIgnored feels a bit vague but I can't think of something better either.

ViktorSimko · 2018-06-21T04:49:38Z

I can also agree with @hartbit .
For me, isIgnored would be expressing enough.
It's a member of WorkingCheckout so I think it's clear that it means "ignored by whatever VCS we are using".

hartbit · 2018-06-21T05:08:31Z

is singular does feel wrong as it checks for multiple files thought: areFilesIgnored(at:)?

ViktorSimko · 2018-06-21T05:13:50Z

Yeah, that's right.
areFilesIgnored(at:) seems to be better.

Add test cases for the inclusion of files in the root directory, for the inclusion of non-source files in source directories and for the exclusion of files by git.

Put parentheses around the functional trailing closures

…ction Pass all the extra file paths to `isGitIgnored` at once to improve performance

ViktorSimko · 2018-06-30T10:54:29Z

I've removed the Git related changes from this.

aciidgh · 2018-06-30T18:18:50Z

Sources/Xcodeproj/generate().swift

+        }
+    }
+
+    if let checkout = workingCheckout {


Hmm, maybe we shouldn't do this unless we have a working checkout because otherwise we will end up adding dot files like .DS_Store

Yeah, I can agree on this, also I think in most cases there will be a working checkout, so requiring it for this feature shouldn't cause many issues.

…kout

aciidgh · 2018-07-04T01:21:26Z

@swift-ci test with toolchain

aciidgh · 2018-07-04T01:30:17Z

Sources/Xcodeproj/generate().swift

+        if !isFile($0) { return false }
+        if let `extension` = $0.extension, SupportedLanguageExtension.validExtensions.contains(`extension`) {
+            return false
+        }


We should also exclude dot files here.

…onSourceFiles Like this, we won't check the extension for dot files unnecessarily

aciidgh · 2018-07-04T16:33:52Z

Ok, this looks good to me! Thanks a lot for all your work here!!

aciidgh · 2018-07-04T16:33:58Z

@swift-ci please smoke test

ViktorSimko · 2018-07-04T21:16:31Z

Thank you for reviewing my code and helping me!

In swiftlang#1507, we started adding "extra" files to the generated Xcode project. This makes some enhancements to that feature: - Introduce a new flag `--skip-extra-files` to go back to the pre-4.2 behaviour (best behaviour) of not including these files at all - Do not add a file reference for "Package.resolved" since this shouldn't be edited by hand and most people also never have to read it - Move the "extra" file references after the products group. This may be debatable, but I think it more clean to have all file references related to the package manifest grouped together at the top instead of having these interspersed throughout the main group.

In #1507, we started adding "extra" files to the generated Xcode project. This makes some enhancements to that feature: - Introduce a new flag `--skip-extra-files` to go back to the pre-4.2 behaviour (best behaviour) of not including these files at all - Do not add a file reference for "Package.resolved" since this shouldn't be edited by hand and most people also never have to read it - Move the "extra" file references after the products group. This may be debatable, but I think it more clean to have all file references related to the package manifest grouped together at the top instead of having these interspersed throughout the main group.

aciidgh reviewed Feb 26, 2018

View reviewed changes

aciidgh reviewed Mar 1, 2018

View reviewed changes

aciidgh added the WIP Work in progress label Mar 1, 2018

ViktorSimko force-pushed the SR-1741 branch 2 times, most recently from c43fab6 to 7b95046 Compare March 3, 2018 21:50

ViktorSimko mentioned this pull request Mar 4, 2018

[SR-7074] Introduce ability to add top level files #1516

Closed

hartbit reviewed Jun 19, 2018

View reviewed changes

aciidgh reviewed Jun 20, 2018

View reviewed changes

ViktorSimko mentioned this pull request Jun 20, 2018

Add isGitIgnored method for checking if a file is ignored by git #1613

Merged

ViktorSimko added 2 commits June 29, 2018 08:43

Add non-source files from source directories

7b39124

ignore files from .gitignore

d5a05cf

ViktorSimko added 5 commits June 30, 2018 08:38

Simplify findNonSourceFiles, fix its documentation

a754741

Check if sources directories exist

5d655cf

Add tests for GenerateXcodeproj

3340415

Add test cases for the inclusion of files in the root directory, for the inclusion of non-source files in source directories and for the exclusion of files by git.

Fix the style of trailing closures

6e2ce11

Put parentheses around the functional trailing closures

Refactor the finding and filtering of non-source files into a new fun…

a2d2ad8

…ction Pass all the extra file paths to `isGitIgnored` at once to improve performance

ViktorSimko force-pushed the SR-1741 branch from 7dfa398 to a2d2ad8 Compare June 30, 2018 08:57

aciidgh reviewed Jun 30, 2018

View reviewed changes

Only add non-source files to Xcode project if there is a working chec…

8e73ea1

…kout

aciidgh reviewed Jul 4, 2018

View reviewed changes

ViktorSimko added 2 commits July 4, 2018 06:47

Exclude dot files in findNonSourceFiles

056e97e

Move the check for dot files before the check for extensions in findN…

82072e9

…onSourceFiles Like this, we won't check the extension for dot files unnecessarily

aciidgh removed the WIP Work in progress label Jul 4, 2018

aciidgh merged commit f98bc3e into swiftlang:master Jul 4, 2018

neonichu mentioned this pull request Aug 30, 2018

Improve handling of "extra" files #1771

Merged

aciidgh mentioned this pull request Jul 4, 2018

[SR-1741] SwiftPM folder references for non-source directories #5363

Closed

		@@ -92,6 +92,13 @@ public class GitRepositoryProvider: RepositoryProvider {
		}
		}

SR-1741: Add non-source files from source directories and root, ignore files from .gitignore #1507

SR-1741: Add non-source files from source directories and root, ignore files from .gitignore #1507

Uh oh!

Conversation

ViktorSimko commented Feb 23, 2018

Uh oh!

aciidgh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giginet commented Mar 4, 2018

Uh oh!

hartbit commented Jun 12, 2018

Uh oh!

ViktorSimko commented Jun 14, 2018

Uh oh!

ViktorSimko commented Jun 14, 2018

Uh oh!

ViktorSimko commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hartbit commented Jun 19, 2018

Uh oh!

ViktorSimko commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorSimko commented Jun 20, 2018

Uh oh!

hartbit commented Jun 20, 2018

Uh oh!

aciidgh commented Jun 20, 2018

Uh oh!

ViktorSimko commented Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hartbit commented Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorSimko commented Jun 21, 2018

Uh oh!

ViktorSimko commented Jun 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciidgh commented Jul 4, 2018

ViktorSimko commented Jun 21, 2018 •

edited

Loading

hartbit commented Jun 21, 2018 •

edited

Loading