Ignore normal directory with dots #397

toshi0383 · 2018-09-13T16:24:05Z

Fixes #396

brentleyjones · 2018-09-13T18:49:51Z

“.bundle”

_{Sent with GitHawk}

yonaskolb

This could potentially break a lot of projects that were relying on directories with an extension to be a file

toshi0383 · 2018-09-14T00:40:36Z

Yea I worry about it too, but if we thought about it at first time, this should be the correct approach, isn't it?

brentleyjones · 2018-09-14T17:18:14Z

Sources/XcodeGenKit/PathKitExtension.swift

+                "xcassets",
+                "complicationset",
+                "appiconset",
+                "xcdatamodeld"].contains(ex)


We should add "bundle" and "framework" to this as well.

Will do👌

I wonder if there is an API to detect this? It seems to be OS defined.

Found an API to get UTI in string. 1e21b1d
It's "public.folder" if it's a normal folder.
lproj appears to be "public.folder" too. It looks problematic, but we're whitelisting lproj, so it wouldn't be a problem.

toshi0383 · 2018-09-15T02:47:53Z

extension: appiconset, uti: public.folder
extension: bundle, uti: com.apple.generic-bundle
extension: complicationset, uti: public.folder
extension: framework, uti: com.apple.framework
extension: imageset, uti: public.folder
extension: lproj, uti: public.folder
extension: xcassets, uti: com.apple.dt.assetcatalog
extension: xcdatamodeld, uti: com.apple.xcode.model.data-version

pepicrft · 2018-09-18T06:45:12Z

Sources/XcodeGenKit/PathKitExtension.swift

+extension Path {
+
+    /// Treat this as a resource instead of a normal directory.
+    var isNonFolderDirectory: Bool {


isNonFolderDirectory sounds a bit ambiguous. What about isGroupDirectory? That's how Xcode names directories in an Xcode project.

Then you could just say !myPath.isGroupDirectory

I understand, but I think it's depends on how you look at the problem.

From Xcode perspective, isGroupDirectory is maybe straight forward, but this specific functionality is seen outside of Xcode too. That's why I've put this as PathKitExtension.

Also, I actually tried to rename it, and ended up with rather complicated code. (Needed additional is a directory, but condition to retrieve non "public.folder" directory.)

let directories = children .filter { $0.isDirectory && $0.isGroupDirectory && $0.extension != "lproj" } .sorted { $0.lastComponent < $1.lastComponent } let filePaths = children .filter { $0.isFile || ($0.isDirectory && !$0.isGroupDirectory) } .sorted { $0.lastComponent < $1.lastComponent }

Therefore, I think it's better to define this way (isNonFolderDirectory) in whitelist manner.

How about isFileDirectory? I agree that isNonFolderDirectory is a bit strange of a name. The actual logic doesn't need to change

Done in 3aac581

pepicrft · 2018-09-18T06:59:49Z

Sources/XcodeGenKit/PathKitExtension.swift

+            .resourceValues(forKeys: [URLResourceKey.typeIdentifierKey])
+            .typeIdentifier {
+            // NOTE: lproj is public.folder
+            return uti != "public.folder"


If someone looks at this code in three months, it's hard to say what it does. Which types of paths am I ignoring with this expression. Maybe it's worth adding a larger comment where you explain the rationale behind this line.

Thanks. Will do. Maybe as a doc-comment.

done in 6933cde

pepicrft · 2018-09-18T07:03:40Z

Yea I worry about it too, but if we thought about it at first time, this should be the correct approach, isn't it?

Not making the most appropriate decision in the past doesn't justify that the "correct" approach should be introduced regardless of it breaking the contract with the projects that are already using the tool. What I would do is:

Add tests that cover the most common file types.
Check whether the behaviour is the same after your change.

If it's not, there's a decision to be made: either introducing a breaking change and going with a major bump, or finding a way to roll this out in a non-breaking manner. @yonaskolb is the person to help you with that.

toshi0383 · 2018-09-18T11:12:37Z

Add tests that cover the most common file types.

Check whether the behaviour is the same after your change.

Thanks, will do👌

toshi0383 · 2018-09-23T01:14:41Z

Just found out how to list all recognized filetypes on mac. It lists all icons for each filetypes too.
https://discussions.apple.com/thread/3117503

I'll collect filetypes from these things.

            CFBundleTypeExtensions =             (
                xcodeproj,
                xcode,
                pbproj
            );
            CFBundleTypeIconFile = "xcode-project_Icon";
            CFBundleTypeName = "Xcode Project";
            CFBundleTypeRole = Editor;
            LSIsAppleDefaultForType = 1;
            LSItemContentTypes =             (
                "com.apple.xcode.project"
            );
            LSTypeIsPackage = 1;
            NSDocumentClass = Xcode3ProjectDocument;
            "_LSIconPath" = "Contents/Resources/xcode-project_Icon.icns";
        },

                conforms to:   com.apple.package, com.apple.dt.interfacebuilder.documen
t.storyboard
                tags:          .storyboardc
        --------------------------------------------------------
        type    id:            54184
                uti:           com.apple.dt.assetcatalog
                description:   Xcode Asset Catalog
                flags:         exported  active  apple-internal  trusted
                icon:
                conforms to:   public.directory
                tags:          .xcassets
        --------------------------------------------------------
        type    id:            54188
                uti:           com.apple.dt.stickers
                description:   Xcode Stickers Catalog
                flags:         exported  active  apple-internal  trusted
                icon:
                conforms to:   public.directory
                tags:          .xcstickers
        --------------------------------------------------------
        type    id:            54192
                uti:           com.apple.interfacebuilder.plugin

As a side note:
I'm not sure if we should depends on this database (which means dynamically executing and parsing this command output to get all these special filetypes). It looks like end-user can update these states using the lsregister command or something else.

toshi0383 · 2018-09-23T13:42:08Z

Tests/XcodeGenKitTests/SourceGeneratorTests.swift

            }

            $0.it("supports frameworks in sources") {
                let directories = """
                Sources:
-                  - Foo.framework
+                  - Foo.framework:
+                    - Foo.swift


*.framework is now required to be a directory, not file.

toshi0383 · 2018-09-23T13:59:01Z

@pepibumur @yonaskolb

Add tests that cover the most common file types.

Added tests in b24a7bf .
File extensions are retrieved by this one-liner.

(for i in $(/System/Library/Frameworks/CoreServices.framework/Versions/A/Frameworks/LaunchServices.framework/Versions/A/Support/lsregister -dump | grep ' \.[a-zA-Z][a-zA-Z]*' | sed -n 's/.*: *$.*$/\1/p' | sort | uniq | sed "s,[ '],,g" | sed 's/,/ /g'); do echo $i; done) | grep '^\.' | grep -v '^\.\.*$' | sort | uniq

(Sorry for poor readability. The original output is not in any standard format and is hard to parse.)

Check whether the behaviour is the same after your change.

The behavior is different for normal directory with dots. They had been treated as .resources BuildPhase (as shown in #396 ), but now as normal directories.
Other things like "special directory with dots" or "normal directory without dots" are not affected.

toshi0383 · 2018-09-23T14:04:22Z

Tests/XcodeGenKitTests/SourceGeneratorTests.swift

+    }
+}
+
+// MARK: Utilities


Moved utility functions below the all test cases. What we are interested in most is the test cases themselves, not utilities.

toshi0383 · 2018-09-23T14:12:53Z

And this is the script to determine if a file extension has a special meaning or not.
https://gist.github.com/toshi0383/41b7d3bcc27ae01246558114da6bcdd7

toshi0383 · 2018-09-26T10:36:32Z

@yonaskolb
Not sure why the test is failing. It passes locally with Xcode9.3 on HighSierra.

yonaskolb · 2018-09-29T06:13:19Z

Sources/XcodeGenKit/PathKitExtension.swift

+            return false
+        }
+
+        if let uti = try! URL(fileURLWithPath: self.string)


I'd like to get the performance tests in (coming shortly) so this can be tested for performance

yonaskolb · 2018-09-29T06:23:21Z

@toshi0383 Not sure why tests are failing either.
Could you add a changelog entry as well detailing the changes and mentioning the breaking change with normal directories with dots not being added to .resources anymore

Conflicts: Tests/Fixtures/TestProject/Project.xcodeproj/project.pbxproj

toshi0383 · 2018-09-30T09:44:52Z

Updated CHANGELOG in 9227503

toshi0383 · 2018-10-01T06:51:52Z

Investigating CI failure. Maybe returned UTI is different on Mac VMs?

toshi0383 · 2018-10-01T07:11:51Z

Path.string: TestDirectory/Sources/Hello.agilekeychain, uti: public.folder

So we need to figure out how to determine if an UTI is from System, or from third party app.

toshi0383 · 2022-10-18T02:36:23Z

Closing for now.
Feel free to take over or reopen!👋🏻

Ignore normal directory names with dots

07fbf83

toshi0383 requested review from brentleyjones and yonaskolb September 13, 2018 16:24

toshi0383 added 4 commits September 14, 2018 01:25

typo

6073ea0

Add assets and imagests

96e9bbb

fix bugs

80e0cbf

Add test case in SourceGeneratorTests

e883566

yonaskolb reviewed Sep 13, 2018

View reviewed changes

toshi0383 mentioned this pull request Sep 14, 2018

Directory name containing dots is unexpectedly added to copy bundle resources build phase #396

Open

brentleyjones reviewed Sep 14, 2018

View reviewed changes

Get UTI using URL's API

1e21b1d

toshi0383 requested review from ryohey, keith and pepicrft September 15, 2018 02:55

pepicrft reviewed Sep 18, 2018

View reviewed changes

toshi0383 added the WIP label Sep 18, 2018

toshi0383 added 5 commits September 23, 2018 14:50

Ignore directories which treated as special file

3433988

Merge remote-tracking branch 'origin/master' into ts-filelikedir

6b30fbd

Add tests for all recognized file extensions

b24a7bf

Add comment and improve code

6933cde

Add MARK

0f8bc82

toshi0383 commented Sep 23, 2018

View reviewed changes

toshi0383 removed the WIP label Sep 23, 2018

toshi0383 commented Sep 23, 2018

View reviewed changes

keith removed their request for review September 24, 2018 21:36

yonaskolb reviewed Sep 29, 2018

View reviewed changes

toshi0383 added 4 commits September 30, 2018 18:01

Merge remote-tracking branch 'origin/master' into ts-filelikedir

d6890dc

Conflicts: Tests/Fixtures/TestProject/Project.xcodeproj/project.pbxproj

Fix conflict

fd74ab1

Rename to isFileDirectory

3aac581

Update CHANGELOG.md

9227503

Potentially fix CI failure

4165ec8

toshi0383 added the WIP label Oct 1, 2018

toshi0383 force-pushed the ts-filelikedir branch from 77ca9b1 to 24fa8ab Compare October 1, 2018 06:59

debug

a6b925a

toshi0383 force-pushed the ts-filelikedir branch from 24fa8ab to a6b925a Compare October 1, 2018 07:05

toshi0383 closed this Oct 18, 2022

toshi0383 deleted the ts-filelikedir branch October 18, 2022 02:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore normal directory with dots #397

Ignore normal directory with dots #397

toshi0383 commented Sep 13, 2018 •

edited

Loading

brentleyjones commented Sep 13, 2018

yonaskolb left a comment

toshi0383 commented Sep 14, 2018

brentleyjones Sep 14, 2018

toshi0383 Sep 14, 2018

brentleyjones Sep 14, 2018

toshi0383 Sep 15, 2018

toshi0383 commented Sep 15, 2018

pepicrft Sep 18, 2018

pepicrft Sep 18, 2018

toshi0383 Sep 18, 2018

yonaskolb Sep 29, 2018

toshi0383 Sep 30, 2018

pepicrft Sep 18, 2018

toshi0383 Sep 18, 2018

toshi0383 Sep 23, 2018

pepicrft commented Sep 18, 2018

toshi0383 commented Sep 18, 2018

toshi0383 commented Sep 23, 2018

toshi0383 Sep 23, 2018

toshi0383 commented Sep 23, 2018

toshi0383 Sep 23, 2018

toshi0383 commented Sep 23, 2018

toshi0383 commented Sep 26, 2018

yonaskolb Sep 29, 2018

yonaskolb commented Sep 29, 2018

toshi0383 commented Sep 30, 2018

toshi0383 commented Oct 1, 2018

toshi0383 commented Oct 1, 2018

toshi0383 commented Oct 18, 2022

Ignore normal directory with dots #397

Ignore normal directory with dots #397

Conversation

toshi0383 commented Sep 13, 2018 • edited Loading

brentleyjones commented Sep 13, 2018

yonaskolb left a comment

Choose a reason for hiding this comment

toshi0383 commented Sep 14, 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

toshi0383 commented Sep 15, 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

pepicrft commented Sep 18, 2018

toshi0383 commented Sep 18, 2018

toshi0383 commented Sep 23, 2018

Choose a reason for hiding this comment

toshi0383 commented Sep 23, 2018

Choose a reason for hiding this comment

toshi0383 commented Sep 23, 2018

toshi0383 commented Sep 26, 2018

Choose a reason for hiding this comment

yonaskolb commented Sep 29, 2018

toshi0383 commented Sep 30, 2018

toshi0383 commented Oct 1, 2018

toshi0383 commented Oct 1, 2018

toshi0383 commented Oct 18, 2022

toshi0383 commented Sep 13, 2018 •

edited

Loading