Fix duplicate localized resource files (#363)

Resolves https://github.com/tuist/tuist/issues/361

### Short description

Localized resources can up getting duplicated within the resources build phase and the project files elements.

The reason this was occurring:

- Give a path to a resource e.g. `/resources/en.lproj/foo.png`
- `/resources/en.lproj/` is used detected
- All files within `/resources/en.lproj/` are enumerated and added 
- When another path to a resource is encountered e.g. `/resources/en.lproj/bar.png`
- The same process is repeated, resulting in duplicate file elements!

### Solution

- Update project files element logic to better accommodate multiple localized files 
- Add a cache to keep track of which build files were added the resources build phase to prevent adding duplicates.
- Improved file element missing file warnings for directories 

e.g. `resources: ["Resources/en.lproj"]` would yield the following warnings:

```
Warning: 'Resources/en.lproj' is a directory, try using: 'Resources/en.lproj/**' to list its files
``` 

### Test Plan

- Verify unit tests pass via `swift tests`
- Verify acceptance tests pass via `bundle rake exec features`
- Manually generate `fixtures/ios_app_with_framework_and_resources` via `tuist generate`
- Verify the generated project includes the localized string files without any duplicates

CHANGELOG.md

@@ -22,6 +22,7 @@ Please, check out guidelines: https://keepachangelog.com/en/1.0.0/
 - Fixing generation failures due to asset catalog & `**/*.png` glob patterns handling https://github.com/tuist/tuist/pull/346 by @kwridan 
 - Supporting bundle target dependencies that reside in different projects (in `TuistGenerator`) https://github.com/tuist/tuist/pull/348 by @kwridan 
 - Fixing header paths including folders and non-header files https://github.com/tuist/tuist/pull/356 by @kwridan
+- Fix duplicate localized resource files https://github.com/tuist/tuist/pull/363 by @kwridan
 
 ## 0.14.0
 

Sources/TuistGenerator/Generator/BuildPhaseGenerator.swift

@@ -171,6 +171,7 @@ final class BuildPhaseGenerator: BuildPhaseGenerating {
                                             fileElements: ProjectFileElements,
                                             pbxproj: PBXProj,
                                             resourcesBuildPhase: PBXResourcesBuildPhase) throws {
+        var buildFilesCache = Set<AbsolutePath>()
         try files.forEach { buildFilePath in
             let pathString = buildFilePath.pathString
             let pathRange = NSRange(location: 0, length: pathString.count)
@@ -185,7 +186,7 @@ final class BuildPhaseGenerator: BuildPhaseGenerating {
                 return
             }
 
-            var element: PBXFileElement?
+            var element: (element: PBXFileElement, path: AbsolutePath)?
 
             if isLocalized {
                 let name = buildFilePath.components.last!
@@ -193,18 +194,18 @@ final class BuildPhaseGenerator: BuildPhaseGenerating {
                 guard let group = fileElements.group(path: path) else {
                     throw BuildPhaseGenerationError.missingFileReference(buildFilePath)
                 }
-                element = group
-
+                element = (group, path)
             } else if !isLproj {
                 guard let fileReference = fileElements.file(path: buildFilePath) else {
                     throw BuildPhaseGenerationError.missingFileReference(buildFilePath)
                 }
-                element = fileReference
+                element = (fileReference, buildFilePath)
             }
-            if let element = element {
-                let pbxBuildFile = PBXBuildFile(file: element)
+            if let element = element, buildFilesCache.contains(element.path) == false {
+                let pbxBuildFile = PBXBuildFile(file: element.element)
                 pbxproj.add(object: pbxBuildFile)
                 resourcesBuildPhase.files?.append(pbxBuildFile)
+                buildFilesCache.insert(element.path)
             }
         }
     }

Sources/TuistGenerator/Generator/ProjectFileElements.swift

@@ -304,11 +304,16 @@ class ProjectFileElements {
 
         // Add the file element
         if isLocalized(path: absolutePath) {
-            addVariantGroup(from: from,
-                            absolutePath: absolutePath,
-                            relativePath: relativePath,
-                            toGroup: toGroup,
-                            pbxproj: pbxproj)
+            // Localized container (e.g. /path/to/en.lproj) we don't add it directly
+            // an element will get added once the next path component is evaluated
+            //
+            // note: assumption here is a path to a nested resource is always provided
+            return (element: toGroup, path: absolutePath)
+        } else if isLocalized(path: from) {
+            // Localized file (e.g. /path/to/en.lproj/foo.png)
+            addLocalizedFile(localizedFile: absolutePath,
+                             toGroup: toGroup,
+                             pbxproj: pbxproj)
             return nil
         } else if isVersionGroup(path: absolutePath) {
             return addVersionGroupElement(from: from,
@@ -335,36 +340,60 @@ class ProjectFileElements {
         }
     }
 
-    func addVariantGroup(from: AbsolutePath,
-                         absolutePath: AbsolutePath,
-                         relativePath _: RelativePath,
-                         toGroup: PBXGroup,
-                         pbxproj: PBXProj) {
-        // /path/to/*.lproj/*
-        absolutePath.glob("*").sorted().forEach { localizedFile in
-            let localizedName = localizedFile.components.last!
+    func addLocalizedFile(localizedFile: AbsolutePath,
+                          toGroup: PBXGroup,
+                          pbxproj: PBXProj) {
+        // e.g.
+        // from: resources/en.lproj/
+        // localizedFile: resources/en.lproj/App.strings
 
-            // Variant group
-            let variantGroupPath = absolutePath.parentDirectory.appending(component: localizedName)
-            var variantGroup: PBXVariantGroup! = elements[variantGroupPath] as? PBXVariantGroup
-            if variantGroup == nil {
-                variantGroup = PBXVariantGroup(children: [], sourceTree: .group, name: localizedName)
-                pbxproj.add(object: variantGroup)
-                toGroup.children.append(variantGroup)
-                elements[variantGroupPath] = variantGroup
-            }
+        // Variant Group
+        let localizedName = localizedFile.basename // e.g. App.strings
+        let localizedContainer = localizedFile.parentDirectory // e.g. resources/en.lproj
+        let variantGroupPath = localizedContainer
+            .parentDirectory
+            .appending(component: localizedName) // e.g. resources/App.strings
 
-            // Localized element
-            let localizedFilePath = "\(absolutePath.components.last!)/\(localizedName)" // e.g: en.lproj/Main.storyboard
-            let lastKnownFileType = Xcode.filetype(extension: localizedName) // e.g. Main.storyboard
-            let name = absolutePath.components.last!.split(separator: ".").first! // e.g. en
-            let localizedFileReference = PBXFileReference(sourceTree: .group,
-                                                          name: String(name),
-                                                          lastKnownFileType: lastKnownFileType,
-                                                          path: localizedFilePath)
-            pbxproj.add(object: localizedFileReference)
-            variantGroup.children.append(localizedFileReference)
+        let variantGroup = addVariantGroup(variantGroupPath: variantGroupPath,
+                                           localizedName: localizedName,
+                                           toGroup: toGroup,
+                                           pbxproj: pbxproj)
+
+        // Localized element
+        addLocalizedFileElement(localizedFile: localizedFile,
+                                variantGroup: variantGroup,
+                                localizedContainer: localizedContainer,
+                                pbxproj: pbxproj)
+    }
+
+    private func addVariantGroup(variantGroupPath: AbsolutePath,
+                                 localizedName: String,
+                                 toGroup: PBXGroup,
+                                 pbxproj: PBXProj) -> PBXVariantGroup {
+        if let variantGroup = elements[variantGroupPath] as? PBXVariantGroup {
+            return variantGroup
         }
+
+        let variantGroup = PBXVariantGroup(children: [], sourceTree: .group, name: localizedName)
+        pbxproj.add(object: variantGroup)
+        toGroup.children.append(variantGroup)
+        elements[variantGroupPath] = variantGroup
+        return variantGroup
+    }
+
+    private func addLocalizedFileElement(localizedFile: AbsolutePath,
+                                         variantGroup: PBXVariantGroup,
+                                         localizedContainer: AbsolutePath,
+                                         pbxproj: PBXProj) {
+        let localizedFilePath = localizedFile.relative(to: localizedContainer.parentDirectory)
+        let lastKnownFileType = Xcode.filetype(extension: localizedFile.basename)
+        let name = localizedContainer.basename.split(separator: ".").first
+        let localizedFileReference = PBXFileReference(sourceTree: .group,
+                                                      name: name.map { String($0) },
+                                                      lastKnownFileType: lastKnownFileType,
+                                                      path: localizedFilePath.pathString)
+        pbxproj.add(object: localizedFileReference)
+        variantGroup.children.append(localizedFileReference)
     }
 
     func addVersionGroupElement(from: AbsolutePath,

Sources/TuistKit/Generator/GeneratorModelLoader.swift

@@ -110,7 +110,11 @@ extension TuistGenerator.FileElement {
                 .filter(includeFiles)
 
             if files.isEmpty {
-                printer.print(warning: "No files found at: \(string)")
+                if fileHandler.isFolder(path.appending(RelativePath(string))) {
+                    printer.print(warning: "'\(string)' is a directory, try using: '\(string)/**' to list its files")
+                } else {
+                    printer.print(warning: "No files found at: \(string)")
+                }
             }
 
             return files

Tests/TuistGeneratorTests/Generator/BuildPhaseGeneratorTests.swift

@@ -162,27 +162,37 @@ final class BuildPhaseGeneratorTests: XCTestCase {
     }
 
     func test_generateResourcesBuildPhase_whenLocalizedFile() throws {
-        let path = AbsolutePath("/en.lproj/Main.storyboard")
-        let target = Target.test(resources: [.file(path: path)])
-        let fileElements = ProjectFileElements()
-        let pbxproj = PBXProj()
-        let group = PBXVariantGroup()
-        pbxproj.add(object: group)
-        fileElements.elements[AbsolutePath("/Main.storyboard")] = group
-        let nativeTarget = PBXNativeTarget(name: "Test")
+        // Given
+        let files: [AbsolutePath] = [
+            "/path/resources/en.lproj/Main.storyboard",
+            "/path/resources/en.lproj/App.strings",
+            "/path/resources/fr.lproj/Main.storyboard",
+            "/path/resources/fr.lproj/App.strings",
+        ]
 
+        let resources = files.map { FileElement.file(path: $0) }
+        let fileElements = createLocalizedResourceFileElements(for: [
+            "/path/resources/Main.storyboard",
+            "/path/resources/App.strings",
+        ])
+
+        let nativeTarget = PBXNativeTarget(name: "Test")
+        let pbxproj = PBXProj()
+
+        // When
         try subject.generateResourcesBuildPhase(path: "/path",
-                                                target: target,
+                                                target: .test(resources: resources),
                                                 graph: Graph.test(),
                                                 pbxTarget: nativeTarget,
                                                 fileElements: fileElements,
                                                 pbxproj: pbxproj)
 
-        let pbxBuildPhase: PBXBuildPhase? = nativeTarget.buildPhases.first
-        XCTAssertNotNil(pbxBuildPhase)
-        XCTAssertTrue(pbxBuildPhase is PBXResourcesBuildPhase)
-        let pbxBuildFile: PBXBuildFile? = pbxBuildPhase?.files?.first
-        XCTAssertEqual(pbxBuildFile?.file, group)
+        // Then
+        let buildPhase = nativeTarget.buildPhases.first
+        XCTAssertEqual(buildPhase?.files?.map { $0.file }, [
+            fileElements.elements["/path/resources/Main.storyboard"],
+            fileElements.elements["/path/resources/App.strings"],
+        ])
     }
 
     func test_generateResourcesBuildPhase_whenCoreDataModel() throws {
@@ -317,4 +327,12 @@ final class BuildPhaseGeneratorTests: XCTestCase {
         })
         return fileElements
     }
+
+    private func createLocalizedResourceFileElements(for files: [AbsolutePath]) -> ProjectFileElements {
+        let fileElements = ProjectFileElements()
+        fileElements.elements = Dictionary(uniqueKeysWithValues: files.map {
+            ($0, PBXVariantGroup())
+        })
+        return fileElements
+    }
 }

Tests/TuistGeneratorTests/Generator/ProjectFileElementsTests.swift

@@ -132,8 +132,7 @@ final class ProjectFileElementsTests: XCTestCase {
     func test_addElement_xcassets() throws {
         // Given
         let element = GroupFileElement(path: "/path/myfolder/resources/assets.xcassets/foo/bar.png",
-                                       group: .group(name: "Project"),
-                                       isReference: true)
+                                       group: .group(name: "Project"))
 
         // When
         try subject.generate(fileElement: element,
@@ -158,8 +157,7 @@ final class ProjectFileElementsTests: XCTestCase {
         ]
         let elements = resouces.map {
             GroupFileElement(path: AbsolutePath($0),
-                             group: .group(name: "Project"),
-                             isReference: true)
+                             group: .group(name: "Project"))
         }
 
         // When
@@ -177,6 +175,44 @@ final class ProjectFileElementsTests: XCTestCase {
         ])
     }
 
+    func test_addElement_lproj_multiple_files() throws {
+        // Given
+        let resouces = try fileHandler.createFiles([
+            "resources/en.lproj/App.strings",
+            "resources/en.lproj/Extension.strings",
+            "resources/fr.lproj/App.strings",
+            "resources/fr.lproj/Extension.strings",
+        ])
+
+        let elements = resouces.map {
+            GroupFileElement(path: $0,
+                             group: .group(name: "Project"),
+                             isReference: true)
+        }
+
+        // When
+        try elements.forEach {
+            try subject.generate(fileElement: $0,
+                                 groups: groups,
+                                 pbxproj: pbxproj,
+                                 sourceRootPath: fileHandler.currentPath)
+        }
+
+        // Then
+        let projectGroup = groups.main.group(named: "Project")
+        XCTAssertEqual(projectGroup?.debugChildPaths, [
+            "resources/App.strings/en",
+            "resources/App.strings/fr",
+            "resources/Extension.strings/en",
+            "resources/Extension.strings/fr",
+        ])
+
+        XCTAssertEqual(projectGroup?.debugVariantGroupPaths, [
+            "resources/App.strings",
+            "resources/Extension.strings",
+        ])
+    }
+
     func test_targetProducts() {
         let target = Target.test()
         let products = subject.targetProducts(target: target).sorted()
@@ -362,32 +398,23 @@ final class ProjectFileElementsTests: XCTestCase {
         XCTAssertEqual(file.sourceTree, .group)
     }
 
-    func test_addVariantGroup() throws {
-        let fileName = "localizable.strings"
-        let dir = try TemporaryDirectory(removeTreeOnDeinit: true)
-        let localizedDir = dir.path.appending(component: "en.lproj")
-        try fileHandler.createFolder(localizedDir)
-        try "test".write(to: localizedDir.appending(component: fileName).url, atomically: true, encoding: .utf8)
-        let from = dir.path.parentDirectory
-        let absolutePath = localizedDir
-        let relativePath = RelativePath("en.lproj")
-        let group = PBXGroup()
+    func test_addLocalizedFile() throws {
+        // Given
         let pbxproj = PBXProj()
-        pbxproj.add(object: group)
-        subject.addVariantGroup(from: from,
-                                absolutePath: absolutePath,
-                                relativePath: relativePath,
-                                toGroup: group,
-                                pbxproj: pbxproj)
-        let variantGroupPath = dir.path.appending(component: fileName)
-        let variantGroup: PBXVariantGroup = subject.group(path: variantGroupPath) as! PBXVariantGroup
-        XCTAssertEqual(variantGroup.name, fileName)
-        XCTAssertEqual(variantGroup.sourceTree, .group)
+        let group = PBXGroup()
+        let file: AbsolutePath = "/path/to/resources/en.lproj/App.strings"
 
-        let fileReference: PBXFileReference? = variantGroup.children.first as? PBXFileReference
-        XCTAssertEqual(fileReference?.name, "en")
-        XCTAssertEqual(fileReference?.sourceTree, .group)
-        XCTAssertEqual(fileReference?.path, "en.lproj/\(fileName)")
+        // When
+        subject.addLocalizedFile(localizedFile: file,
+                                 toGroup: group,
+                                 pbxproj: pbxproj)
+
+        // Then
+        let variantGroup = group.children.first as? PBXVariantGroup
+        XCTAssertEqual(variantGroup?.name, "App.strings")
+        XCTAssertNil(variantGroup?.path)
+        XCTAssertEqual(variantGroup?.children.map { $0.name }, ["en"])
+        XCTAssertEqual(variantGroup?.children.map { $0.path }, ["en.lproj/App.strings"])
     }
 
     func test_addVersionGroupElement() throws {
@@ -527,4 +554,18 @@ private extension PBXGroup {
             }
         }
     }
+
+    /// Retuns all the child variant groups (recursively)
+    var debugVariantGroupPaths: [String] {
+        return children.flatMap { (element: PBXFileElement) -> [String] in
+            switch element {
+            case let group as PBXVariantGroup:
+                return [group.nameOrPath]
+            case let group as PBXGroup:
+                return group.debugVariantGroupPaths.map { group.nameOrPath + "/" + $0 }
+            default:
+                return []
+            }
+        }
+    }
 }

Tests/TuistKitTests/Generator/GeneratorModelLoaderTest.swift

@@ -269,70 +269,6 @@ class GeneratorModelLoaderTest: XCTestCase {
         XCTAssertEqual(model.projects, [])
     }
 
-    func test_loadWorkspace_withInvalidFilePaths() throws {
-        // Given
-        let path = fileHandler.currentPath
-        try fileHandler.createFolders([
-            "Documentation",
-        ])
-
-        let manifests = [
-            path: WorkspaceManifest.test(name: "SomeWorkspace",
-                                         projects: [],
-                                         additionalFiles: [
-                                             "Documentation/**/*.md",
-                                         ]),
-        ]
-
-        let manifestLoader = createManifestLoader(with: manifests)
-        let subject = GeneratorModelLoader(fileHandler: fileHandler,
-                                           manifestLoader: manifestLoader,
-                                           manifestTargetGenerator: manifestTargetGenerator,
-                                           printer: printer)
-
-        // When
-        let model = try subject.loadWorkspace(at: path)
-
-        // Then
-        XCTAssertEqual(printer.printWarningArgs, [
-            "No files found at: Documentation/**/*.md",
-        ])
-        XCTAssertEqual(model.additionalFiles, [])
-    }
-
-    func test_loadWorkspace_withInvalidFolderReferencePaths() throws {
-        // Given
-        let path = fileHandler.currentPath
-        try fileHandler.createFiles([
-            "README.md",
-        ])
-
-        let manifests = [
-            path: WorkspaceManifest.test(name: "SomeWorkspace",
-                                         projects: [],
-                                         additionalFiles: [
-                                             .folderReference(path: "Documentation"),
-                                             .folderReference(path: "README.md"),
-                                         ]),
-        ]
-
-        let manifestLoader = createManifestLoader(with: manifests)
-        let subject = GeneratorModelLoader(fileHandler: fileHandler,
-                                           manifestLoader: manifestLoader,
-                                           manifestTargetGenerator: manifestTargetGenerator,
-                                           printer: printer)
-
-        // When
-        let model = try subject.loadWorkspace(at: path)
-
-        // Then
-        XCTAssertEqual(printer.printWarningArgs, [
-            "Documentation does not exist",
-            "README.md is not a directory - folder reference paths need to point to directories",
-        ])
-        XCTAssertEqual(model.additionalFiles, [])
-    }
-
     func test_settings() throws {
         // Given
         let debug = ConfigurationManifest(settings: ["Debug": "Debug"], xcconfig: "debug.xcconfig")
@@ -474,6 +410,88 @@ class GeneratorModelLoaderTest: XCTestCase {
         XCTAssertEqual(GeneratorModelLoaderError.missingFile("/missing/path").description, "Couldn't find file at path '/missing/path'")
     }
 
+    func test_fileElement_warning_withDirectoryPathsAsFiles() throws {
+        // Given
+        let path = fileHandler.currentPath
+        try fileHandler.createFiles([
+            "Documentation/README.md",
+            "Documentation/USAGE.md",
+        ])
+
+        let manifest = ProjectDescription.FileElement.glob(pattern: "Documentation")
+
+        // When
+        let model = TuistGenerator.FileElement.from(manifest: manifest,
+                                                    path: path,
+                                                    fileHandler: fileHandler,
+                                                    printer: printer,
+                                                    includeFiles: { !self.fileHandler.isFolder($0) })
+
+        // Then
+        XCTAssertEqual(printer.printWarningArgs, [
+            "'Documentation' is a directory, try using: 'Documentation/**' to list its files",
+        ])
+        XCTAssertEqual(model, [])
+    }
+
+    func test_fileElement_warning_withMisingPaths() throws {
+        // Given
+        let path = fileHandler.currentPath
+        let manifest = ProjectDescription.FileElement.glob(pattern: "Documentation/**")
+
+        // When
+        let model = TuistGenerator.FileElement.from(manifest: manifest,
+                                                    path: path,
+                                                    fileHandler: fileHandler,
+                                                    printer: printer)
+
+        // Then
+        XCTAssertEqual(printer.printWarningArgs, [
+            "No files found at: Documentation/**",
+        ])
+        XCTAssertEqual(model, [])
+    }
+
+    func test_fileElement_warning_withInvalidFolderReference() throws {
+        // Given
+        let path = fileHandler.currentPath
+        try fileHandler.createFiles([
+            "README.md",
+        ])
+
+        let manifest = ProjectDescription.FileElement.folderReference(path: "README.md")
+
+        // When
+        let model = TuistGenerator.FileElement.from(manifest: manifest,
+                                                    path: path,
+                                                    fileHandler: fileHandler,
+                                                    printer: printer)
+
+        // Then
+        XCTAssertEqual(printer.printWarningArgs, [
+            "README.md is not a directory - folder reference paths need to point to directories",
+        ])
+        XCTAssertEqual(model, [])
+    }
+
+    func test_fileElement_warning_withMissingFolderReference() throws {
+        // Given
+        let path = fileHandler.currentPath
+        let manifest = ProjectDescription.FileElement.folderReference(path: "Documentation")
+
+        // When
+        let model = TuistGenerator.FileElement.from(manifest: manifest,
+                                                    path: path,
+                                                    fileHandler: fileHandler,
+                                                    printer: printer)
+
+        // Then
+        XCTAssertEqual(printer.printWarningArgs, [
+            "Documentation does not exist",
+        ])
+        XCTAssertEqual(model, [])
+    }
+
     // MARK: - Helpers
 
     func createManifestLoader(with projects: [AbsolutePath: ProjectDescription.Project]) -> GraphManifestLoading {

features/generate.feature

@@ -76,6 +76,10 @@ Scenario: The project is an iOS application that has resources (ios_app_with_fra
     Then the product 'App.app' with destination 'Debug-iphoneos' contains resource 'Examples/list.json'
     Then the product 'App.app' with destination 'Debug-iphoneos' contains resource 'Assets.car'
     Then the product 'App.app' with destination 'Debug-iphoneos' contains resource 'resource.txt'
+    Then the product 'App.app' with destination 'Debug-iphoneos' contains resource 'en.lproj/App.strings'
+    Then the product 'App.app' with destination 'Debug-iphoneos' contains resource 'en.lproj/Greetings.strings'
+    Then the product 'App.app' with destination 'Debug-iphoneos' contains resource 'fr.lproj/App.strings'
+    Then the product 'App.app' with destination 'Debug-iphoneos' contains resource 'fr.lproj/Greetings.strings'
     Then the product 'App.app' with destination 'Debug-iphoneos' contains resource 'resource_without_extension'
     Then the product 'App.app' with destination 'Debug-iphoneos' does not contain resource 'do_not_include.dat'
 

fixtures/ios_app_with_framework_and_resources/App/Project.swift

@@ -14,6 +14,7 @@ let project = Project(
                 "Resources/**/*.png",
                 "Resources/*.xcassets",
                 "Resources/**/*.txt",
+                "Resources/**/*.strings",
                 "Resources/resource_without_extension",
                 .folderReference(path: "Examples")
             ],

fixtures/ios_app_with_framework_and_resources/App/Resources/en.lproj/Greetings.strings

@@ -0,0 +1,2 @@
+
+"morning" = "Good Morning";

fixtures/ios_app_with_framework_and_resources/App/Resources/fr.lproj/Greetings.strings

@@ -0,0 +1,2 @@
+
+"morning" = "Bonjour";