Adds FilesLinter that makes sure all required files are available #6069

mustiikhalil · 2024-03-12T22:14:20Z

Short description 📝

Adds FilesLinter that makes sure that all non-glob files would would present warnings

Resolves #5552

How to test the changes locally 🧐

The command should be useable through out the entire project, however to test this following the steps below would be the best approach:

Go to fixtures that's missing resources, or sources.
Running tuist generate --no-open should flush a lot of warnings which is to be expected

Output

...
The following warnings need attention:
 · No files found at: /ios_app_with_framework_and_missing_resources/App/Resources/**/*.png
 · No files found at:/ios_app_with_framework_and_missing_resources/App/Resources/*.xcassets
 · No files found at:/ios_app_with_framework_and_missing_resources/StaticFramework/Resources/images/t.txt
 · The target StaticFramework doesn't contain source files.
 · No files found at: /ios_app_with_framework_and_missing_resources/StaticFramework/Resources/images/**/*.png
 ...

Running tuist generate --no-open should flush non-glob warnings as shown below

Output

�[33m · No resources found at: Resources/images/t.txt
 · No files found at: Sources/App+internals.swift
 · The target StaticFramework doesn't contain source files.�[0m

Contributor checklist ✅

The code has been linted using run mise run lint:fix
The change is tested via unit testing or acceptance testing, or both
The title of the PR is formulated in a way that is usable as a changelog entry
In case the PR introduces changes that affect users, the documentation has been updated

Reviewer checklist ✅

The code architecture and patterns are consistent with the rest of the codebase
Reviewer has checked that, if needed, the PR includes the label changelog:added, changelog:fixed, or changelog:changed, and the title is usable as a changelog entry

mustiikhalil · 2024-03-12T22:15:19Z

I wonder

how or where should I document the changes?
And if unit tests are actually needed in this case?

danieleformichelli · 2024-03-12T22:28:36Z

I don't think we should blindly suppress warnings, but rather allow suppressing a specific instance of it.
Otherwise you can just ignore the output and you are good 😁

mustiikhalil · 2024-03-12T22:43:49Z

@danieleformichelli okay, then how do we identify or specify which warnings a developer would want to suppress? what I mean by that is do we separate the types of warnings within tuist into different domains and then run it somewhat like this --suppress-warnings lint missing-files?

danieleformichelli · 2024-03-12T22:54:53Z

@danieleformichelli okay, then how do we identify or specify which warnings a developer would want to suppress? what I mean by that is do we separate the types of warnings within tuist into different domains and then run it somewhat like this --suppress-warnings lint missing-files?

For this specific case, I think it should be a target property whether to ignore if it has ick m invalid sources or not

mustiikhalil · 2024-03-13T07:16:55Z

Wouldn't that go against why we would want to add suppress the warnings? For example, on local development machines we would want to suppress warnings, but not on the CI.

danieleformichelli · 2024-03-13T08:28:46Z

Wouldn't that go against why we would want to add suppress the warnings? For example, on local development machines we would want to suppress warnings, but not on the CI.

When have we decided that it was the why? :)
I would suggest discussing the matter on slack or a github discussion before jumping into implementation 🙏

My PoV is that ususally it's a good warning. There might be some cases (like the template mentioned in the issue) where it might be annoying, hence we should allow to disable it only for targets created from that template

mustiikhalil · 2024-03-13T08:48:50Z

Sounds good, i'll move this discussion into slack later today! thanks for your input

fixtures/ios_app_with_framework_and_missing_resources/App/Config/App-Info.plist

fortmarek

Thanks a lot for the PR! However, I think we should move the linting from the loading phase to the generation phase instead to align with other linters we already have in the codebase.

fortmarek · 2024-03-18T07:53:48Z

Sources/TuistLoader/Models+ManifestMappers/CopyFileElement+ManifestMapper.swift

+                    if !path.pathString.isGlobComponent {
+                        // FIXME: This should be done in a linter.
+                        logger.warning("No files found at: \(path.pathString)")
+                    }


Why have we not moved this to the new linter?

Code wise this just needs to be removed, the linter already does this

fortmarek · 2024-03-18T07:54:16Z

Tests/TuistLoaderTests/Linters/FilesLinterTests.swift

+
+@testable import TuistLoader
+
+class FilesLinterTests: XCTestCase {


Suggested change

class FilesLinterTests: XCTestCase {

final class FilesLinterTests: XCTestCase {

fortmarek · 2024-03-18T07:54:33Z

Sources/TuistLoader/Linter/FilesLinter.swift

+    func lint(project: LoadedWorkspace, convertedProjects: [TuistGraph.Project]) -> [LintingIssue]
+}
+
+public class FilesLinter: FilesLinting {


Suggested change

public class FilesLinter: FilesLinting {

public final class FilesLinter: FilesLinting {

fortmarek · 2024-03-18T07:56:06Z

Tests/TuistLoaderTests/Linters/FilesLinterTests.swift

+        let results = subject.lint(project: workspace, convertedProjects: [graphProject])
+
+        // Then
+        XCTAssertTrue(results.contains(LintingIssue(


You can use XCTContainsLintingIssue

I would also assert the number of issues

fortmarek · 2024-03-18T07:56:39Z

Tests/TuistLoaderTests/Linters/FilesLinterTests.swift

Can we also test that a glob for which no files exist does not throw a warning?

fortmarek · 2024-03-18T07:59:13Z

Sources/TuistKit/Utils/ManifestGraphLoader.swift

+        let filesLintingIssues = filesLinter.lint(
+            project: allManifests,
+            convertedProjects: projectsModels
+        )
+        filesLintingIssues.printWarningsIfNeeded()


Why is this linted here and not along with all the other linters we have here? This could probably be added as a TargetLinter. I don't really see a reason why this needs to be linted during the load phase.

Yeah, so i tried to looking into that, however it seems that we cant really put it there since we require the allManifests and that's being fetched within the same function. and the linter itself compares the paths between, the generated targets from the allManifests to the actual allManifests

Or do you think there is a better option there @fortmarek?

Considering the need "I need linters to know whether a file has been derived from a glob", you need:

A way to express that

Use that information from the linters that @fortmarek pointed out.

We already have a module that represents a path. You can extend it to include information about whether it came from a glob:

public enum FileElement: Equatable, Hashable, Codable { case file(path: AbsolutePath, metadata: Set<FileElementMetadata>) case folderReference(path: AbsolutePath, metadata: Set<FileElementMetadata>) } // Somewhere in the mapping logic. FileElement.path("....", metadata: [.fromGlob])

Okay nice then, I'll take a look at it and see if that's going to work as expected

I've been wondering, do we need the extra metadata? If we decide that we won't show any warnings when no files match a glob but we still want to show a warning for individual files, can't we only lint the individual files exist? As we don't add the parent glob directory if the glob doesn't match anything.

can't we only lint the individual files exist?

That's the idea, but how do you know at the linter level if a file or not was hardcoded by the user or resolved from a glob without passing that information when the models are mapped from ProjectDescription to TuistGraph?

When we resolve a glob, we then pass on only files that the file system can find. How would a non-existing file from a glob be added as a regular file?

I'll resume the work on this weekend, or the weekend after. I've been super busy lately.

Hey, If possible, I would love to get some feedback regarding the approach that was taken before adding tests and basically moving forward with this PR.

pepicrft

Thanks a lot for taking the lead here @mustiikhalil. I think we should move this to the existing linters, and make any necessary changes to allow that solution.

mustiikhalil · 2024-04-11T15:55:43Z

Sources/TuistGenerator/Linter/TargetLinter.swift

+        if target.supportsResources {
+            let files = target.copyFiles.flatMap(\.files)
+


We can lint based on glob, and folder reference too, but for now I made it simple for the sake of reviewing it

fortmarek · 2024-04-16T07:56:59Z

Sources/TuistGraph/Models/Target.swift

+    public var sourcesPaths: [AbsolutePath]
    public var resources: ResourceFileElements
+    public var resourcesPaths: [AbsolutePath]
    public var copyFiles: [CopyFilesAction]
+    public var copyFilesPaths: [AbsolutePath]


Do we actually need to track these paths in extra properties? Can't we lint that the files added do exist based on directly the sources, resources, and copyFiles properties?

Well, i tried to make it work but couldnt since we kinda need to find a point of reference. For example, /app/sources/text.swift will not show up in the sources if its not within the directory. So we kinda need to store the paths to compare them later on in the linter.

There should be a place where we skip adding files to sources when they don't exist. Maybe we can try including them? Would be interesting to see what happens as that should be a valid operation in Xcode.

@fortmarek so I updated the code to remove all of those and simply just allow tuist to append all the files into Xcode. And then use the TargetLinter to display if there are any missing folders or files.

If this is the way to progress, I will add the tests and finalize the code for it

mustiikhalil · 2024-05-03T14:55:09Z

@fortmarek if you have time to review it so we can move forward towards getting this PR merged.

Creates a file linter that check if all non-glob Files within the targets are available within the generated targets and project Moves File validation into the TargetLinter as discussed

mustiikhalil requested a review from pepicrft March 12, 2024 22:16

mustiikhalil self-assigned this Mar 12, 2024

mustiikhalil force-pushed the issue-5552 branch 2 times, most recently from 5dda4a9 to 979cf0b Compare March 16, 2024 14:30

mustiikhalil marked this pull request as ready for review March 16, 2024 16:23

mustiikhalil force-pushed the issue-5552 branch from 796e917 to 59db266 Compare March 16, 2024 16:27

mustiikhalil changed the title ~~Adds Suppress warnings flag to TuistCommand~~ Adds FilesLinter that makes sure all required files are available Mar 16, 2024

danieleformichelli reviewed Mar 16, 2024

View reviewed changes

fixtures/ios_app_with_framework_and_missing_resources/App/Config/App-Info.plist Outdated Show resolved Hide resolved

danieleformichelli approved these changes Mar 16, 2024

View reviewed changes

mustiikhalil force-pushed the issue-5552 branch from 30d7b15 to a2f8682 Compare March 17, 2024 08:15

fortmarek reviewed Mar 18, 2024

View reviewed changes

pepicrft requested changes Mar 18, 2024

View reviewed changes

mustiikhalil force-pushed the issue-5552 branch 3 times, most recently from c9547a7 to adc7d4b Compare April 11, 2024 15:52

mustiikhalil commented Apr 11, 2024

View reviewed changes

fortmarek reviewed Apr 16, 2024

View reviewed changes

fortmarek mentioned this pull request Apr 23, 2024

Skip warnings for External Dependencies on tuist generate #6223

Closed

mustiikhalil force-pushed the issue-5552 branch from adc7d4b to 1a1f041 Compare April 30, 2024 01:32

mustiikhalil force-pushed the issue-5552 branch from 1a1f041 to b21df9f Compare May 3, 2024 14:55

Adds FilesLinter that makes sure all required files are available

dafe0e5

Creates a file linter that check if all non-glob Files within the targets are available within the generated targets and project Moves File validation into the TargetLinter as discussed

mustiikhalil force-pushed the issue-5552 branch from b21df9f to dafe0e5 Compare May 15, 2024 05:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds FilesLinter that makes sure all required files are available #6069

Adds FilesLinter that makes sure all required files are available #6069

mustiikhalil commented Mar 12, 2024 •

edited

mustiikhalil commented Mar 12, 2024

danieleformichelli commented Mar 12, 2024

mustiikhalil commented Mar 12, 2024

danieleformichelli commented Mar 12, 2024

mustiikhalil commented Mar 13, 2024

danieleformichelli commented Mar 13, 2024 •

edited

mustiikhalil commented Mar 13, 2024

fortmarek left a comment

fortmarek Mar 18, 2024

mustiikhalil Apr 11, 2024

fortmarek Mar 18, 2024

fortmarek Mar 18, 2024

fortmarek Mar 18, 2024

fortmarek Mar 18, 2024

fortmarek Mar 18, 2024

mustiikhalil Mar 18, 2024

mustiikhalil Mar 18, 2024

pepicrft Mar 18, 2024

mustiikhalil Mar 18, 2024

fortmarek Mar 23, 2024

pepicrft Mar 25, 2024

fortmarek Mar 25, 2024

mustiikhalil Apr 4, 2024

mustiikhalil Apr 11, 2024

pepicrft left a comment

mustiikhalil Apr 11, 2024

fortmarek Apr 16, 2024 •

edited

mustiikhalil Apr 16, 2024

fortmarek Apr 16, 2024

mustiikhalil Apr 30, 2024 •

edited

mustiikhalil commented May 3, 2024


		@testable import TuistLoader

		class FilesLinterTests: XCTestCase {

	class FilesLinterTests: XCTestCase {
	final class FilesLinterTests: XCTestCase {

	public class FilesLinter: FilesLinting {
	public final class FilesLinter: FilesLinting {

		if target.supportsResources {
		let files = target.copyFiles.flatMap(\.files)

Adds FilesLinter that makes sure all required files are available #6069

Are you sure you want to change the base?

Adds FilesLinter that makes sure all required files are available #6069

Conversation

mustiikhalil commented Mar 12, 2024 • edited

Short description 📝

How to test the changes locally 🧐

Output

Output

Contributor checklist ✅

Reviewer checklist ✅

mustiikhalil commented Mar 12, 2024

danieleformichelli commented Mar 12, 2024

mustiikhalil commented Mar 12, 2024

danieleformichelli commented Mar 12, 2024

mustiikhalil commented Mar 13, 2024

danieleformichelli commented Mar 13, 2024 • edited

mustiikhalil commented Mar 13, 2024

fortmarek 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

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortmarek Apr 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustiikhalil Apr 30, 2024 • edited

Choose a reason for hiding this comment

mustiikhalil commented May 3, 2024

mustiikhalil commented Mar 12, 2024 •

edited

danieleformichelli commented Mar 13, 2024 •

edited

fortmarek Apr 16, 2024 •

edited

mustiikhalil Apr 30, 2024 •

edited