Add API to search for files in a group with path. Handles non-unique filenames in groups #671

itsthejb · 2022-03-30T12:19:15Z

Resolves #670

Short description 📝

Add path based accessor for files in a group

Solution 📦

Simply generate a full path, using the available source root, so it's possible to filter on a complete abs/relative path, rather than just assuming that the filename is enough

Implementation 👩‍💻👨‍💻

Step 1: Add unit test
Step 2: Implement file find method
Step 3: Deprecate the name method

itsthejb · 2022-03-30T12:20:03Z

Sources/XcodeProj/Objects/Files/PBXGroup.swift

@@ -120,12 +120,26 @@ public extension PBXGroup {
    ///
    /// - Parameter name: file name.
    /// - Returns: file with the given name contained in the given parent group.
+    @available(*, deprecated, message: "Please use file(with:relativeTo:). This method assumes all names in the group are unique")


Might be going a little far! But arguably the functionality is too broad, and unclear to the user

Perhaps updating the doc comments here may suffice? It can clarify the usage and point to the other method for cases where searching by path is needed.

Sounds good. Let me take care of that right away

itsthejb · 2022-03-30T12:20:54Z

Sources/XcodeProj/Objects/Files/PBXGroup.swift

+    /// - Parameter basePath: a base path to search from.
+    /// - Returns: file with the given absolute path contained in the given parent group.
+    func file(with path: Path, relativeTo basePath: Path) -> PBXFileReference? {
+        let normalized = path.normalized(relativeTo: basePath)


1: Use the basePath to generate a complete path to path; normalization takes care of various path traversal possibilities

itsthejb · 2022-03-30T12:21:21Z

Sources/XcodeProj/Objects/Files/PBXGroup.swift

+    func file(with path: Path, relativeTo basePath: Path) -> PBXFileReference? {
+        let normalized = path.normalized(relativeTo: basePath)
+        return children
+            .first { $0.normalizedPath(relativeTo: basePath) == normalized } as? PBXFileReference


2: Do likewise for each file in the group; if the normalized result matches, then it is at the same location on the disk

itsthejb · 2022-03-30T12:21:50Z

Tests/XcodeProjTests/Objects/Files/PBXGroupTests.swift

+        XCTAssertEqual(
+            group.file(with: "file", relativeTo: rootPath),
+            try XCTUnwrap(filesToRefs[file1])
+        )
+        XCTAssertEqual(
+            group.file(with: "file", relativeTo: rootPath + "subdir"),
+            try XCTUnwrap(filesToRefs[file2])
+        )
+        XCTAssertEqual(
+            group.file(with: "subdir/file", relativeTo: rootPath),
+            try XCTUnwrap(filesToRefs[file2])
+        )


Could add further cases here, they are mostly to communicate the basic use cases

awesome thanks for adding this!

One aspect to consider is, typically the paths dealt with within project files are relative rather absolute paths either to source root, or to their parent group (this is so projects are portable / can work on different machines / directories etc...)

It's worth exploring the behaviour when that is the case.

Yes indeed; I verified this in our parent project. The paths just need to share a common root, that doesn't need to be /, and often it would be the root of the project (i.e. .). I'll adjust this test to include that

codecov-commenter · 2022-03-31T06:41:58Z

Codecov Report

Merging #671 (a3188c3) into main (6f2c1a7) will decrease coverage by 0.00%.
The diff coverage is 41.17%.

@@            Coverage Diff             @@
##             main     #671      +/-   ##
==========================================
- Coverage   84.75%   84.74%   -0.01%     
==========================================
  Files         157      157              
  Lines        8987     8995       +8     
==========================================
+ Hits         7617     7623       +6     
- Misses       1370     1372       +2

Impacted Files	Coverage Δ
Sources/XcodeProj/Objects/Files/PBXGroup.swift	`78.57% <41.17%> (-0.28%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f2c1a7...a3188c3. Read the comment docs.

kwridan

thanks for taking the time to contribute this @itsthejb

kwridan · 2022-03-31T06:16:18Z

Sources/XcodeProj/Objects/Files/PBXGroup.swift

@@ -120,12 +120,26 @@ public extension PBXGroup {
    ///
    /// - Parameter name: file name.
    /// - Returns: file with the given name contained in the given parent group.
+    @available(*, deprecated, message: "Please use file(with:relativeTo:). This method assumes all names in the group are unique")


Perhaps updating the doc comments here may suffice? It can clarify the usage and point to the other method for cases where searching by path is needed.

kwridan · 2022-03-31T06:47:54Z

Tests/XcodeProjTests/Objects/Files/PBXGroupTests.swift

+        XCTAssertEqual(
+            group.file(with: "file", relativeTo: rootPath),
+            try XCTUnwrap(filesToRefs[file1])
+        )
+        XCTAssertEqual(
+            group.file(with: "file", relativeTo: rootPath + "subdir"),
+            try XCTUnwrap(filesToRefs[file2])
+        )
+        XCTAssertEqual(
+            group.file(with: "subdir/file", relativeTo: rootPath),
+            try XCTUnwrap(filesToRefs[file2])
+        )


awesome thanks for adding this!

One aspect to consider is, typically the paths dealt with within project files are relative rather absolute paths either to source root, or to their parent group (this is so projects are portable / can work on different machines / directories etc...)

It's worth exploring the behaviour when that is the case.

itsthejb · 2022-03-31T10:48:17Z

Sources/XcodeProj/Objects/Files/PBXGroup.swift

+    /// - Note: Performs a linear search based on the file's name _only_. To search for a specific file
+    /// based on file-system location, use the `file(with path:)` method.


Note on the limitations, instead of deprecation

itsthejb · 2022-03-31T10:50:54Z

Sources/XcodeProj/Objects/Files/PBXGroup.swift

+        func absolutePath(for fileRef: PBXFileElement) -> Path? {
+            try? fileRef.fullPath(sourceRoot: ".")
+        }
+
+        guard let groupAbsPath = absolutePath(for: self) else { return nil }
+        let fileNormalizedAbsolutePath = (groupAbsPath + path).normalize()
+        return children.first {
+            guard let candidateAbsPath = absolutePath(for: $0) else { return false }
+            return candidateAbsPath.normalize() == fileNormalizedAbsolutePath
+        } as? PBXFileReference


The algorithm has now quite importantly changed, and is much improved, but requires some feedback:

I realised that the functionality already exists (fullPath(sourceRoot:)) to recursively generate an absolute path for all files involved. Passing . to represent "from current dir" seems to work just fine for "give me a path from the root of the file system"..?

Therefore a relative directory is not required by the caller: simply convert the argument path, and all group paths to normalized absolute paths, and search for that

This way, it's guaranteed that any kind of path (relative, absolute, traversals) work as input, since absolute paths are always generated internally as the file system "source of truth"

I think using "." for the sourceRoot is wrong and will only work correctly for cases when the working dir is the same as project location. In other cases it will work in unpredictable way e.g. if you will have absolute file reference in project and a relative one which will resolve in incorrect way they will not be comparable. I'd advise to pass a real sourceRoot to the method.

…on-unique names

itsthejb · 2022-03-31T11:03:46Z

Tests/XcodeProjTests/Objects/Files/PBXGroupTests.swift

+        assert(
+            filePath: Path(UUID().uuidString),
+            hasReference: nil
+        )


This got a little involved, but the basic idea is to recreate Xcode-like scenarios:

You have a projectDir which can be located anywhere on disk

You have a group, which has some path relative to parents, or absolute, etc. (as expected in Xcode)

The group contains files that have paths that likewise relate to the group, or absolute etc., might have messy traversals to parent directories

Specifics of these are irrelevant, since internally we generate those normalized absolute paths

itsthejb · 2022-03-31T11:05:59Z

Ok! This has now actually changed somewhat, and much improved (I hope!). The only real question is whether or not fullPath(sourceRoot:) might have some unexpected results in some situations; it seems not, though...

itsthejb · 2022-03-31T11:08:23Z

Tests/XcodeProjTests/Objects/Files/PBXGroupTests.swift

+            filePath: groupDir + subDirectoryName + ".." + fileName,
+            hasReference: fileReferenceFor[file1Path]
+        )
+        assert(
+            filePath: groupDir + "foo/.." + subDirectoryName + fileName,


Testing traversals to parents

damirdavletov · 2022-03-31T13:48:51Z

Sources/XcodeProj/Objects/Files/PBXGroup.swift

+        func absolutePath(for fileRef: PBXFileElement) -> Path? {
+            try? fileRef.fullPath(sourceRoot: ".")
+        }
+
+        guard let groupAbsPath = absolutePath(for: self) else { return nil }
+        let fileNormalizedAbsolutePath = (groupAbsPath + path).normalize()
+        return children.first {
+            guard let candidateAbsPath = absolutePath(for: $0) else { return false }
+            return candidateAbsPath.normalize() == fileNormalizedAbsolutePath
+        } as? PBXFileReference


I think using "." for the sourceRoot is wrong and will only work correctly for cases when the working dir is the same as project location. In other cases it will work in unpredictable way e.g. if you will have absolute file reference in project and a relative one which will resolve in incorrect way they will not be comparable. I'd advise to pass a real sourceRoot to the method.

itsthejb · 2022-03-31T15:30:19Z

Tests/XcodeProjTests/Objects/Files/PBXGroupPathSearchTests.swift

+import XcodeProj
+import XCTest
+
+final class PBXGroupPathSearchTests: XCTestCase {


Split into own file. Could combine again, but it did get a little messy

itsthejb · 2022-03-31T15:32:22Z

Tests/XcodeProjTests/Objects/Files/PBXGroupPathSearchTests.swift

+    func test_whenFilesHaveAbsoluteSourceTree_thenCanBeFoundByPath() throws {
+        try checkAssertions(for: .absolute)
+    }
+
+    func test_whenFilesHaveGroupSourceTree_thenCanBeFoundByPath() throws {
+        try checkAssertions(for: .group)
+    }
+    func test_whenFilesHaveSourceRootSourceTree_thenCanBeFoundByPath() throws {
+        try checkAssertions(for: .sourceRoot)


Refactored to test all the PBXSourceTree types that are actually used in the fullPath(sourceRoot:) method

As far as I can see, as long as the ($SRCROOT) is passed to the find method, everything works as expected

itsthejb · 2022-03-31T15:34:12Z

Sources/XcodeProj/Objects/Files/PBXGroup.swift

+    ///   - sourceRoot: path to project's source root.
+    /// - Returns: an existing reference to that file, or `nil` if not found
+    func file(with path: Path, sourceRoot: Path) -> PBXFileReference? {
+        func absolutePath(for fileRef: PBXFileElement) -> Path? {
+            try? fileRef.fullPath(sourceRoot: sourceRoot)


Going with the convention of the other methods, then, and "just pass in $(SRCROOT) here and everything should work". Tests seem to show it does

itsthejb · 2022-03-31T16:24:19Z

Sources/XcodeProj/Objects/Files/PBXGroup.swift

-        if override, let existingFileReference = try projectObjects.fileReferences.first(where: {
-            // Optimization: compare lastComponent before fullPath compare
-            guard let fileRefPath = $0.value.path else {
-                return try filePath == $0.value.fullPath(sourceRoot: sourceRoot)
-            }
-            let fileRefLastPathComponent = fileRefPath.split(separator: "/").last!
-            if filePath.lastComponent == fileRefLastPathComponent {
-                return try filePath == $0.value.fullPath(sourceRoot: sourceRoot)
-            }
-            return false
-        }) {
+        if override, let existingFileReference = try self.existingFileReference(
+            from: projectObjects,
+            with: filePath,
+            sourceRoot: sourceRoot


Having spent some time going round and round on this, trying to create comprehensive tests, whilst still being slightly confused by how PBXSourceTrees actually work, I finally realised that the functionality needed is actually already here. It's only necessary to extract is and create a public driver method @kwridan @damirdavletov.

Therefore I decided not to assume too much about my knowledge of this lib, or PBXProj in general, and simply assume that this functionality works and expose it as we require. Seems much simpler. I can't guarantee it previously worked, but it should not break anything either...?

itsthejb · 2022-03-31T16:26:19Z

Sources/XcodeProj/Objects/Files/PBXGroup.swift

+private extension PBXGroup {
+
+    func existingFileReference(
+        from objects: PBXObjects? = nil,


Since the original already accessed a PBXObjects, I thought to at least keep that as an optional arg. It seems to be just a constant reference, but playing it safe here:

XcodeProj/Sources/XcodeProj/Objects/Project/PBXObject.swift

Lines 75 to 81 in 6943d5c

func objects() throws -> PBXObjects {

guard let objects = reference.objects else {

let objectType = String(describing: type(of: self))

throw PBXObjectError.orphaned(type: objectType, reference: reference.value)

}

return objects

}

itsthejb · 2022-03-31T16:29:11Z

Sources/XcodeProj/Objects/Files/PBXGroup.swift

+        try (objects ?? self.objects()).fileReferences.first(where: {
+            // Optimization: compare lastComponent before fullPath compare
+            guard let fileRefPath = $0.value.path else {
+                return try filePath == $0.value.fullPath(sourceRoot: sourceRoot)
+            }
+            let fileRefLastPathComponent = fileRefPath.split(separator: "/").last!
+            if filePath.lastComponent == fileRefLastPathComponent {
+                return try filePath == $0.value.fullPath(sourceRoot: sourceRoot)
+            }
+            return false
+        })


Moved from

XcodeProj/Sources/XcodeProj/Objects/Files/PBXGroup.swift

Lines 190 to 200 in 6943d5c

if override, let existingFileReference = try projectObjects.fileReferences.first(where: {

// Optimization: compare lastComponent before fullPath compare

guard let fileRefPath = $0.value.path else {

return try filePath == $0.value.fullPath(sourceRoot: sourceRoot)

}

let fileRefLastPathComponent = fileRefPath.split(separator: "/").last!

if filePath.lastComponent == fileRefLastPathComponent {

return try filePath == $0.value.fullPath(sourceRoot: sourceRoot)

}

return false

}) {

kwridan

Thanks for the follow ups and updates @itsthejb 👍

Having dug in a little further the exposed method does introduce some API awkwardness 🤔 - arguably existing in the addFile method.

Taking the following simple demo project

  func testFileWithPath() throws {
    // Given
    let xodeProj = try XcodeProj(path: demoPath)
    let mainGroup = try XCTUnwrap(try xodeProj.pbxproj.rootGroup())
    let sourcesGroup = try XCTUnwrap(mainGroup.children.first(where: { $0.path == "Sources" }) as? PBXGroup)

    // When
    //   Expected 👍
    let readmeReference1 = mainGroup.file(with: "README.md", sourceRoot: ".")
    let helloSwift1 = sourcesGroup.file(with: "Sources/Hello.swift", sourceRoot: ".")
    let helloSwift2 = sourcesGroup.file(with: demoPathSourceRoot + "Sources/Hello.swift", sourceRoot: demoPathSourceRoot)

    //   Possibly expected 🤔
    let helloSwift3 = mainGroup.file(with: "Sources/Hello.swift", sourceRoot: ".")

    //   Not expected ... 🙈
    let readmeReference2 = sourcesGroup.file(with: "README.md", sourceRoot: ".")
    let readmeReference3 = sourcesGroup.file(with: demoPathSourceRoot + "README.md", sourceRoot: demoPathSourceRoot)

    // Then
    XCTAssertNotNil(readmeReference1)
    XCTAssertNotNil(helloSwift1)
    XCTAssertNotNil(helloSwift2)
    XCTAssertNotNil(helloSwift3)
    XCTAssertNil(readmeReference2) // ❌ Fails
    XCTAssertNil(readmeReference3) // ❌ Fails
}

The last few cases seem a bit odd, where we are asking the Sources group for README.md (which shouldn't exist within it) - yet it returns the reference from its parent. Perhaps I am not using the API correctly?

I don't have great any suggestions yet sadly - will have another look through the code to get a better understanding.

cc: @tuist/core incase any of you have further insights.

Thanks again @itsthejb

itsthejb · 2022-04-01T08:53:18Z

@kwridan Thanks for investigating further. To be honest, going deeper into this in order to validate it worked as expected for all cases was beginning to stretch my understanding of PBXSourceTrees, which I've only semi-understanded over the years working in Xcode. I was also trying to set up a whole suite of unit tests (which are still in previous commits), but started chasing my tail and getting a headache... 🤯

So, when I realised there was already an implementation in there, I went with the approach of exposing that, rather than assuming DIY could be any better (probably worse)

Actually our original use case was avoiding duplicate files in groups, and this whole exercise has taught me that addFile(...) already has some form of this...

github-actions · 2023-06-27T02:10:49Z

Hola 👋,

We want to let you know that your pull request has been marked as stale. It seems that there hasn't been any activity or updates on it for a while.

If you're still interested in having this pull request merged or reviewed, please provide any necessary updates or address any feedback that may have been given. We would be happy to continue the review process and consider merging it into the main branch.

However, if this pull request is no longer a priority or if you've decided to take a different approach, please let us know so we can close it accordingly.

Thank you for your understanding and contribution.

github-actions · 2023-07-02T02:13:09Z

Hola 👋,

We want to let you know that we have decided to close your pull request #456 due to prolonged inactivity. Despite the initial interest and efforts put into the pull request, we haven't seen any updates or responses for a considerable period of time.

We understand that circumstances change and priorities shift, which may have led to this inactivity. If you still wish to contribute or have further discussions on this feature or bug fix, please don't hesitate to reopen the pull request and engage with the community.

We appreciate your understanding and the time you invested in submitting the pull request. Your contributions are valuable, and we hope to collaborate with you on future endeavors.

Thank you.

itsthejb commented Mar 30, 2022

View reviewed changes

itsthejb changed the title ~~Add API to search for files in a group with relative path, handling n…~~ Add API to search for files in a group with path. Handles non-unique filenames in groups Mar 30, 2022

damirdavletov approved these changes Mar 30, 2022

View reviewed changes

kwridan reviewed Mar 31, 2022

View reviewed changes

itsthejb force-pushed the file-search-by-path branch from 625f61f to 8acf6f4 Compare March 31, 2022 10:47

itsthejb commented Mar 31, 2022

View reviewed changes

jcrookebumble added 2 commits March 31, 2022 12:00

Add API to search for files in a group with relative path, handling n…

4bbcf1d

…on-unique names

Remove deprecation, improve docs

8cd16c9

itsthejb force-pushed the file-search-by-path branch from a37d710 to e743f56 Compare March 31, 2022 11:01

itsthejb commented Mar 31, 2022

View reviewed changes

Simplify API, always consider absolute paths internally

51abda8

itsthejb force-pushed the file-search-by-path branch from e743f56 to 51abda8 Compare March 31, 2022 11:07

itsthejb commented Mar 31, 2022

View reviewed changes

damirdavletov suggested changes Mar 31, 2022

View reviewed changes

Refactor tests to test all possible sourceRoot types

2ad12d4

itsthejb commented Mar 31, 2022

View reviewed changes

jcrookebumble added 3 commits March 31, 2022 16:46

Wip

87d18b9

Tests

e3b1fe1

Just extract existing functionality

a3188c3

itsthejb force-pushed the file-search-by-path branch from 9f543f7 to a3188c3 Compare March 31, 2022 16:21

itsthejb commented Mar 31, 2022

View reviewed changes

damirdavletov approved these changes Mar 31, 2022

View reviewed changes

kwridan reviewed Mar 31, 2022

View reviewed changes

github-actions bot added the stale label Jun 27, 2023

github-actions bot closed this Jul 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API to search for files in a group with path. Handles non-unique filenames in groups #671

Add API to search for files in a group with path. Handles non-unique filenames in groups #671

itsthejb commented Mar 30, 2022 •

edited

Loading

itsthejb Mar 30, 2022

kwridan Mar 31, 2022

itsthejb Mar 31, 2022

itsthejb Mar 30, 2022

itsthejb Mar 30, 2022

itsthejb Mar 30, 2022

kwridan Mar 31, 2022

itsthejb Mar 31, 2022

codecov-commenter commented Mar 31, 2022 •

edited

Loading

kwridan left a comment

kwridan Mar 31, 2022

kwridan Mar 31, 2022

itsthejb Mar 31, 2022

itsthejb Mar 31, 2022 •

edited

Loading

damirdavletov Mar 31, 2022

itsthejb Mar 31, 2022

itsthejb commented Mar 31, 2022

itsthejb Mar 31, 2022

damirdavletov Mar 31, 2022

itsthejb Mar 31, 2022

itsthejb Mar 31, 2022

itsthejb Mar 31, 2022

itsthejb Mar 31, 2022

itsthejb Mar 31, 2022

itsthejb Mar 31, 2022

kwridan left a comment

itsthejb commented Apr 1, 2022

github-actions bot commented Jun 27, 2023

github-actions bot commented Jul 2, 2023

		/// - Note: Performs a linear search based on the file's name _only_. To search for a specific file
		/// based on file-system location, use the `file(with path:)` method.

	func objects() throws -> PBXObjects {
	guard let objects = reference.objects else {
	let objectType = String(describing: type(of: self))
	throw PBXObjectError.orphaned(type: objectType, reference: reference.value)
	}
	return objects
	}

	if override, let existingFileReference = try projectObjects.fileReferences.first(where: {
	// Optimization: compare lastComponent before fullPath compare
	guard let fileRefPath = $0.value.path else {
	return try filePath == $0.value.fullPath(sourceRoot: sourceRoot)
	}
	let fileRefLastPathComponent = fileRefPath.split(separator: "/").last!
	if filePath.lastComponent == fileRefLastPathComponent {
	return try filePath == $0.value.fullPath(sourceRoot: sourceRoot)
	}
	return false
	}) {

Add API to search for files in a group with path. Handles non-unique filenames in groups #671

Add API to search for files in a group with path. Handles non-unique filenames in groups #671

Conversation

itsthejb commented Mar 30, 2022 • edited Loading

Short description 📝

Solution 📦

Implementation 👩‍💻👨‍💻

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

codecov-commenter commented Mar 31, 2022 • edited Loading

Codecov Report

kwridan 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

itsthejb Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsthejb commented Mar 31, 2022

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

kwridan left a comment

Choose a reason for hiding this comment

itsthejb commented Apr 1, 2022

github-actions bot commented Jun 27, 2023

github-actions bot commented Jul 2, 2023

itsthejb commented Mar 30, 2022 •

edited

Loading

codecov-commenter commented Mar 31, 2022 •

edited

Loading

itsthejb Mar 31, 2022 •

edited

Loading