Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions Sources/XcodeProj/Objects/Files/PBXGroup.swift
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ public extension PBXGroup {
}

/// Returns the file in the group with the given name.
/// - 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.
Comment on lines +120 to +121
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note on the limitations, instead of deprecation

///
/// - Parameter name: file name.
/// - Returns: file with the given name contained in the given parent group.
Expand All @@ -126,6 +128,27 @@ public extension PBXGroup {
.first(where: { $0.name == name })
}

/// Returns the file in the group with the given path, based on locations on-disk.
/// - Note: Paths function as-in Xcode; with respect to the group's `PBXSourceTree`.
/// Internally, identity is calculated based on normalized absolute paths for all files involved; i.e. actual
/// location in the file system.
///
/// - Parameters:
/// - path: a file path to search for.
/// - Returns: an existing reference to that file, or `nil` if not found
func file(with path: Path) -> PBXFileReference? {
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
Copy link
Author

@itsthejb itsthejb Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/// Creates a group with the given name and returns it.
///
/// - Parameters:
Expand Down
71 changes: 71 additions & 0 deletions Tests/XcodeProjTests/Objects/Files/PBXGroupTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,77 @@ final class PBXGroupTests: XCTestCase {
XCTAssertEqual(file!.sourceTree, .developerDir)
}

func test_whenSearchingForFilesByPath_thenNormalizedAbsolutePathsAreUsedInternallyForUniqueness() throws {
let project = makeEmptyPBXProj()
let subDirectoryName = UUID().uuidString

func prepareDirectories() throws -> (projectDir: Path, groupDir: Path, group: PBXGroup) {
let absolutePath = try Path.uniqueTemporary()
let splitSize = 2
let projectDir = Path(components: absolutePath.components[0..<absolutePath.components.count-splitSize])
let groupDir = Path(components: absolutePath.components.reversed()[0..<splitSize].reversed())
XCTAssertEqual(projectDir + groupDir, absolutePath)

let subDir = absolutePath + subDirectoryName
try subDir.mkdir()

let group = PBXGroup(children: [], sourceTree: .group, name: "group", path: ".")
let parent = PBXGroup(children: [group], sourceTree: .absolute, name: "parent", path: projectDir.string)
project.add(object: parent)
project.add(object: group)

return (projectDir, groupDir, group)
}

let (projectDir, groupDir, theGroup) = try prepareDirectories()

let fileName = UUID().uuidString
let file1Path = groupDir + fileName
let file2Path = groupDir + subDirectoryName + fileName

let fileReferenceFor = try [file1Path, file2Path]
.reduce([Path: PBXFileReference]()) { map, filePath in
let absolutePath = projectDir + filePath
try Data().write(to: absolutePath.url)
let file = try theGroup.addFile(
at: absolutePath,
sourceTree: .group,
sourceRoot: projectDir)

return map.merging([filePath: file]) { _, new in new }
}

func assert(filePath: Path, hasReference: PBXFileReference?, line: UInt = #line) {
let actual = theGroup.file(with: filePath)
if let expected = hasReference {
XCTAssertEqual(actual, expected, file: #file, line: line)
} else {
XCTAssertNil(actual, file: #file, line: line)
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing traversals to parents

hasReference: fileReferenceFor[file2Path]
)
assert(
filePath: Path(UUID().uuidString),
hasReference: nil
)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

private func makeEmptyPBXProj() -> PBXProj {
PBXProj(
rootObject: nil,
Expand Down