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 support for copy files phase #2077

Merged
merged 15 commits into from
Dec 5, 2020
Merged

Conversation

hebertialmeida
Copy link
Contributor

@hebertialmeida hebertialmeida commented Nov 27, 2020

Resolves #2075

Short description 📝

This adds the ability to copy files as a copy files phase.

Solution 📦

Copy the files and create references similiar to resources and add a PBXCopyFilesBuildPhase. One issue is that the packages are not "recognized" on Tuist, so I had to add some validations to check the UTI of a file in order to add the custom packages as whole file instead of its internal file list.

Implementation 👩‍💻👨‍💻

I've reused the implementation of resources for some steps or I've tried to follow the same pattern. One thing I considered is to add a static func for the CopyFilesAction instead of using init, just like TargetActions.

Sample usage:

Target(
    name: "App",
    platform: .macOS,
    product: .app,
    bundleId: "io.tuist.App",
    copyFiles: [
        .sharedSupport(
            name: "Copy Templates",
            subpath: "Templates",
            files: ["/SharedSupport/Templates/**"]
        ),
        .resources(
            name: "Copy Fonts",
            subpath: "Fonts",
            files: ["/Fonts/**"]
        )
    ]
)
  • Validate api design
  • Add tests
  • Add fixture
  • Update documentation
  • Update CHANGELOG

Copy link
Member

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I have some initial remarks, maybe adding a fixture for the new API would help me understand some steps you have taken (especially the UTI handling)

Sources/ProjectDescription/CopyFilesAction.swift Outdated Show resolved Hide resolved
Sources/ProjectDescription/CopyFilesAction.swift Outdated Show resolved Hide resolved
Sources/ProjectDescription/Target.swift Outdated Show resolved Hide resolved
Sources/TuistGenerator/Generator/BuildPhaseGenerator.swift Outdated Show resolved Hide resolved
Sources/TuistSupport/Extensions/AbsolutePath+Extras.swift Outdated Show resolved Hide resolved
@hebertialmeida
Copy link
Contributor Author

Adding a little bit more of context to this.

On iOS apps everything is bundled under Copy Bundle Resources and the files you add for the app will be inside the app bundle. On macOS apps things are a little bit different, for example custom fonts have to be added to a custom folder via copy files phase and referred on .plist file using the key ATSApplicationFontsPath, not as simple as on iOS. Adding support or that will enable these apps to copy files/executables to the proper paths, things like themes, templates, fonts...

Many times these files are packages, basically a folder with a custom extension like .pages, .sketch but they are recognized by the OS as a single file because these apps register a UTI for that extension.

That's why I believe having the name property is important so you see a can organized list and make sense of what is each phase. In addition to that I think naming it CopyFilesPhase makes more sense than BundledFiles. In some places I've used the word Action but that's I'm not sure about that...

This is an example of my app:
image

Apple Pages also do the same thing:
image

https://help.apple.com/xcode/mac/10.2/#/dev50bab713d

@hebertialmeida
Copy link
Contributor Author

Thanks for doing this! I have some initial remarks, maybe adding a fixture for the new API would help me understand some steps you have taken (especially the UTI handling)

Will do that!

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

It's looking great @hebertialmeida. Very solid work and consistent with the rest of the codebase 👏. I'm looking forward to seeing this feature on Tuist.
One more suggestion that I'd make is to make sure to update the CHANGELOG and the documentation website.

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Nice work on this @hebertialmeida - thanks for adding this!

@fortmarek and @pepibumur have covered most points, one addition to help us test this locally and show case this feature is to introduce a new fixture in fixtures/ that exercises the new additions.

Sources/TuistGenerator/Generator/BuildPhaseGenerator.swift Outdated Show resolved Hide resolved
@hebertialmeida hebertialmeida marked this pull request as ready for review December 2, 2020 05:50
@hebertialmeida
Copy link
Contributor Author

I'm seeing the linter fail in lines I haven't touched, is that normal? Should I just fix it?

Copy link
Collaborator

@kwridan kwridan 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 seeing the linter fail in lines I haven't touched, is that normal? Should I just fix it?

I think the linter flags any issues on the file level rather than the diff - should be ok to fix it. There are a few rake tasks to run the same checks as CI

bundle exec rake style_correct

Sources/TuistGenerator/Generator/BuildPhaseGenerator.swift Outdated Show resolved Hide resolved
Moved cleanPackages to ManifestMapper to avoid inconsistency  between `ProjectFileElements` and `BuildPhaseGenerator` as per @kwridan’s suggestion
Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Thanks for all the updates @hebertialmeida - the PR is looking great!

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Thank you very much @hebertialmeida for addressing the comments. There's only one more thing and we can merge the PR afterward, adding documentation. Since we are changing the public API that users have access to, I'd recommend updating this page to indicate that targets have now an attribute, and what the attribute is for.

@pepicrft
Copy link
Contributor

pepicrft commented Dec 4, 2020

@allcontributors
Copy link
Contributor

@pepibumur

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

@pepicrft
Copy link
Contributor

pepicrft commented Dec 4, 2020

@all-contributors add @hebertialmeida for code

@allcontributors
Copy link
Contributor

@pepibumur

I've put up a pull request to add @hebertialmeida! 🎉

@hebertialmeida
Copy link
Contributor Author

@pepibumur @fortmarek @kwridan thanks for the guidance and @natanrolnik for helping me build the project! I have updated the documentation and will merge that after all checks pass.

@hebertialmeida
Copy link
Contributor Author

I can't merge it, so please merge it when you can...

@pepicrft pepicrft merged commit 211dd8a into tuist:main Dec 5, 2020
@hebertialmeida hebertialmeida deleted the copy-files branch December 6, 2020 03:05
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.

RFC: Add support for copy files phase
4 participants