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.

Sign up for GitHub

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

Jump to bottom

Wrong source file filtering when source directory contains a subdirectory with name ends with Target.validSourceExtensions #6186

Open
kyounh12 opened this issue Apr 14, 2024 · 5 comments · May be fixed by #6198
Open

Wrong source file filtering when source directory contains a subdirectory with name ends with Target.validSourceExtensions #6186

kyounh12 opened this issue Apr 14, 2024 · 5 comments · May be fixed by #6198
Labels
type:bug Something isn't working

Comments

@kyounh12
Copy link

kyounh12 commented Apr 14, 2024 •
edited

What happened?

Adding https://github.com/approvals/ApprovalTests.Swift as an external dependency via Tuist/Package.swift causes tuist generate to fail with error below.

/Tuist/.build/checkouts/ApprovalTests.Swift/ApprovalTests.Swift/Approvals.swift to a build phase that hasn't been added to the project.

The package has a subdirectory named ApprovalTests.Swift under source root and Tuist considers this subdirectory as one of the source files. This is because Tuist filters source files with case insensitive path.extension compare and the directory name ApprovalTests.Swift meets this requirement.

Set(paths)
    .subtracting(excluded)
    .filter { path in
        guard let `extension` = path.extension else { return false }
        return Target.validSourceExtensions
            .contains(where: { $0.caseInsensitiveCompare(`extension`) == .orderedSame })
    }
    .forEach { sourceFiles[$0] = SourceFile(
        path: $0,
        compilerFlags: source.compilerFlags,
        codeGen: source.codeGen,
        compilationCondition: source.compilationCondition
    ) }

This leads to an unintended early return in ProjectFileElement.addElement function since the path Tuist/.build/checkouts/ApprovalTests.Swift/ApprovalTests.Swift has been already added to the elements dictionary as a source file. And thus, the source files under ApprovalTests.Swift directory are being ignored and not added to the elements dictionary.

@discardableResult func addElement(
    relativePath: RelativePath,
    isLeaf: Bool,
    from: AbsolutePath,
    toGroup: PBXGroup,
    pbxproj: PBXProj
) -> (element: PBXFileElement, path: AbsolutePath)? {
    let absolutePath = from.appending(relativePath)
    
    if elements[absolutePath] != nil {
        return (element: elements[absolutePath]!, path: from.appending(relativePath))
    }
...

Adding a guard statement to filter out paths that are folders solves the problem but I'm not sure if this is the best place to filter invalid source file paths.

.filter { path in
    guard let `extension` = path.extension else { return false }
    guard !FileHandler.shared.isFolder(path) else { return false } // Adding this line solves the problem
    return Target.validSourceExtensions
        .contains(where: { $0.caseInsensitiveCompare(`extension`) == .orderedSame })
}

How do we reproduce it?

  1. Add ApprovalTest.Swift package in Tuist/Package.swift
.package(url: "https://github.com/approvals/ApprovalTests.Swift.git", branch: "master")
  1. Add ApprovalTest.Swift as an external dependency to one of project targets.
  2. Run tuist install
  3. Run tuist generate

Error log

/Tuist/.build/checkouts/ApprovalTests.Swift/ApprovalTests.Swift/Approvals.swift to a build phase that hasn't been added to the project.

macOS version

14.4.1

Tuist version

4.9.0

Xcode version

15.0
@kyounh12 kyounh12 added the type:bug Something isn't working label Apr 14, 2024
@fortmarek
Copy link
Member

Hey 👋

Yeah, that looks like a valid issue (I'm still being surprised every day by some of the decisions of package authors). Would you want to take a look?

I wouldn't fix this at the ProjectFileElements level but instead wherever we expand the source file glob, we should do a case-sensitive comparison. Before we do that, I'd double check how vanilla SPM works if a package has an actual Swift file called "MyFile.Swift`. Does it pick up? If so, then instead of doing a case sensitive comparison, we would need to check if a given path is a file or a directory.

@danieleformichelli
Copy link
Collaborator

Hey 👋

Yeah, that looks like a valid issue (I'm still being surprised every day by some of the decisions of package authors). Would you want to take a look?

I wouldn't fix this at the ProjectFileElements level but instead wherever we expand the source file glob, we should do a case-sensitive comparison. Before we do that, I'd double check how vanilla SPM works if a package has an actual Swift file called "MyFile.Swift`. Does it pick up? If so, then instead of doing a case sensitive comparison, we would need to check if a given path is a file or a directory.

Can't we check whether it's a file or a directory rather than relying on case?
not 100% sure, but I think Xcode/SPM also accepts upper case, at least for some extensions

@fortmarek
Copy link
Member

Can't we check whether it's a file or a directory rather than relying on case? not 100% sure, but I think Xcode/SPM also accepts upper case, at least for some extensions

Yeah, we can, that's what I said we could also do 😄 I'm not sure how fast the isDirectory operation is because we would technically need to run it for every file, so even if it's fast, it could compound. But maybe it's fast enough that it wouldn't add to the generation time.

If SPM/Xcode pick up files with .Swift extension, then we can't rely on case-sensitive compare of the extension, yeah.

@kyounh12
Copy link
Author

kyounh12 commented Apr 15, 2024 •
edited

스크린샷 2024-04-15 오후 5 43 27

Looks like vanilla SPM accepts uppercased .Swift extension. So now we should check if the path is directory or a file.

I'm not sure which place would be the best to run the check.

  1. Add an extra guard statement below Target+Core.swift L:85 to filter out paths that are directories.
.filter { path in
    guard let `extension` = path.extension else { return false }
    guard !FileHandler.shared.isFolder(path) else { return false } // Adding this line solves the problem
    return Target.validSourceExtensions
        .contains(where: { $0.caseInsensitiveCompare(`extension`) == .orderedSame })
}
  1. Add behavior: Glob.Behavior parameter to throwingGlob function and pass a behavior with includesDirectoriesInResults set to false when getting paths from glob execution. ( Target+Core.swift L:75 )

I tested both implementations and all of them worked as expected

@fortmarek
Copy link
Member

I would probably do it directly in Target+Core.swift instead of providing a new option in the glob behavior.

@kyounh12 kyounh12 linked a pull request Apr 16, 2024 that will close this issue
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type:bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix source file filtering logic to filter out paths that are directories. kyounh12/tuist
3 participants
@danieleformichelli @fortmarek @kyounh12