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

Fix: lproj directories not being detected as resources #4507

Closed
wants to merge 2 commits into from
Closed

Fix: lproj directories not being detected as resources #4507

wants to merge 2 commits into from

Conversation

ffittschen
Copy link
Contributor

Resolves #4505
Request for comments document (if applies): -

Short description 📝

Adds the lproj extension to the validFolderExtensions so that the isResource(path:) will return true instead of false when it's input is a lproj directory.

Question for the maintainers: Is the addition in the tests enough, or should I also add some lproj to any fixtures to assert that a ...Resources target is being generated?

How to test the changes locally 🧐

  1. Use the sample project attached to Resources bundle not generated for SPM dependency #4505
  2. Run tuist fetch && tuist generate
  3. Open the AppCenter project in Tuist > Dependencies > SwiftPackageManager > .build > checkouts
  4. See that Tuist generated the AppCenterDistributeResources target

Checklist ✅

  • The code architecture and patterns are consistent with the rest of the codebase.
  • The changes have been tested following the documented guidelines.
  • The PR includes the label changelog:added, changelog:fixed, or changelog:changed, whenever it should be included in the “Added”, “Fixed” or “Changed” section of the CHANGELOG. Note: when included in the CHANGELOG, the title of the PR will be used as entry, please make sure it is clear and suitable.
  • In case the PR introduces changes that affect users, the documentation has been updated.
  • In case the PR introduces changes that affect how the cache artifact is generated starting from the TuistGraph.Target, the Constants.cacheVersion has been updated.

Copy link
Collaborator

@danieleformichelli danieleformichelli left a comment

Choose a reason for hiding this comment

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

I'm not sure it's the right fix, because AFAIK adding lproj folder as resources works fine in general, and the issue is only when they are added as automatic resources from SPM, so there might be something different in that path to be investigated

@ffittschen
Copy link
Contributor Author

Fair enough. I'll try to find out if we could mimic the workaround explained in #4435 (comment), i.e. assuming the Package.swift file has resources: [.process("Resources"] if it in fact does not, but a Resources directory exists.

@danieleformichelli
Copy link
Collaborator

Fair enough. I'll try to find out if we could mimic the workaround explained in #4435 (comment), i.e. assuming the Package.swift file has resources: [.process("Resources"] if it in fact does not, but a Resources directory exists.

the logic is already there, but it looks like the lproj are handled by a different logic WRT other resources, and that logic is missing somehow for targets coming from Dependencies.swift

@danieleformichelli
Copy link
Collaborator

This should be the required fix: #4532

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resources bundle not generated for SPM dependency
2 participants