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

Support for tvOS Top Shelf Extensions #2793

Merged
merged 15 commits into from Jun 8, 2021
Merged

Support for tvOS Top Shelf Extensions #2793

merged 15 commits into from Jun 8, 2021

Conversation

rmnblm
Copy link
Contributor

@rmnblm rmnblm commented Apr 8, 2021 •
edited

Resolves #2791

Short description 📝

As discussed in #2791, this PR uncomments everything related to .tvExtension.

Checklist ✅

  • The code architecture and patterns are consistent with the rest of the codebase.
  • The changes have been tested following the documented guidelines.
  • The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
  • In case the PR introduces changes that affect users, the documentation has been updated.

@fortmarek
Copy link
Member

Hey @rmnblm 👋

Thanks for the PR - moving the discussion from the issue here.

I uncommented .tvExtension. Running tuist generate works fine, no errors whatsoever. However, the extension target is not added as target to the tvOS target. Any ideas what's missing?

My first guess is here. Generally, you should be fine by looking at where eg watch2Extension is used and make the behavior for tvOS extension similar.

Also, could you modify ios_app_with_extensions fixture to contain tvOS extension? Read more about our acceptance tessts here

pepicrft
pepicrft reviewed Apr 8, 2021
View reviewed changes
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.

Hey @rmnblm. Thanks a lot for taking on the task of adding support for tvOS. As @fortmarek mentioned, I'd add an acceptance test that ensures that we can compile a TV app with an extension and framework, and library dependencies. I'd also update the CHANGELOG to list your changes.

@pepicrft
Copy link
Contributor

pepicrft commented Apr 8, 2021

@all-contributors add @rmnblm for code

@allcontributors allcontributors bot mentioned this pull request Apr 8, 2021
Merged
@allcontributors
Copy link
Contributor

@pepibumur

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

@rmnblm rmnblm mentioned this pull request May 5, 2021
Closed
@rmnblm
Copy link
Contributor Author

rmnblm commented May 5, 2021

@fortmarek I finally found some time to work on this. During my implementation, I found a rather small bug in the XcodeProj package. Please take a look here.

Running the current state of implementation causes the top shelf to crash with the message:

NSExtensionPrincipalClass product_module_name.ContentProvider must implement at least one public protocol

Hence, the PR in the XcodeProj package must be merged before merging this one here. I will work on the tests now and change the state from Draft to Merge later.

So long,
Roman

@rmnblm
Copy link
Contributor Author

rmnblm commented May 5, 2021

I added a new tvos_app_with_extensions fixture to projects/tuist/features/generate-4.feature which basically creates a new tvOS project with a top shelf extension.

Running the acceptance test is successful:

./fourier test tuist acceptance projects/tuist/features/generate-4.feature:47

@rmnblm rmnblm marked this pull request as ready for review May 5, 2021 15:54
fortmarek
fortmarek reviewed May 6, 2021
View reviewed changes
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 the updates @rmnblm! The code seems solid but we'll have to wait 'till this PR gets merged, however

@pepicrft pepicrft added this to the 2.0 milestone May 17, 2021
@rmnblm
Copy link
Contributor Author

rmnblm commented Jun 1, 2021

Hi @fortmarek

I made changes to this PR according to tuist/XcodeProj#609 (comment). I intentionally left out the tvIntentsExtension since, for my current use case, I only need the ability to generate a top shelf.

I hope this is fine.

Cheers,
Roman

@fortmarek fortmarek requested a review from kwridan June 1, 2021 19:37
fortmarek
fortmarek approved these changes Jun 1, 2021
View reviewed changes
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.

Small nits but otherwise seems solid to me, thanks for the contribution @rmnblm 👏

Sources/ProjectDescription/Product.swift Show resolved Hide resolved
Sources/TuistGraph/Models/Product.swift Show resolved Hide resolved
@rmnblm rmnblm mentioned this pull request Jun 2, 2021
Open
@rmnblm
Copy link
Contributor Author

rmnblm commented Jun 2, 2021 •
edited

Added an issue to implement .tvIntentsExtension in the future.

@rmnblm rmnblm changed the title Support for tvOS app extensions Support for tvOS Top Shelf Extensions Jun 2, 2021
kwridan
kwridan reviewed Jun 2, 2021
View reviewed changes
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 👍 thanks for working on this @rmnblm!

Left some minor suggestions and bits I spotted which don't match Xcode's defaults.

Sources/TuistGenerator/Linter/GraphLinter.swift Show resolved Hide resolved
Sources/TuistGenerator/Linter/TargetLinter.swift Show resolved Hide resolved
Sources/TuistCore/ValueGraph/ValueGraphTraverser.swift Outdated Show resolved Hide resolved
Sources/TuistGenerator/Generator/InfoPlistContentProvider.swift Show resolved Hide resolved
projects/tuist/fixtures/tvos_app_with_extensions/TopShelfExtension/ContentProvider.swift Show resolved Hide resolved
@rmnblm
Copy link
Contributor Author

rmnblm commented Jun 7, 2021

I implemented all your suggestions @kwridan. I think this time it is ready to merge, what do you think? 😊

kwridan
kwridan approved these changes Jun 8, 2021
View reviewed changes
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.

Awesome, thanks for the updates @rmnblm!

@fortmarek fortmarek merged commit 27734a0 into tuist:main Jun 8, 2021
mollyIV added a commit to mollyIV/tuist that referenced this pull request Jun 17, 2021
@mollyIV
Merge branch 'main' into add-skip-ui-tests-flag-to-test-command
3539c9e 
* main: (95 commits)
  fix typo on tuist graph command option --skip-test-targets
  Bump normalize-url from 4.5.0 to 4.5.1 in /projects/docs (tuist#3068)
  [Dependencies.swift] Generate `DependenciesGraph` for `Carthage` dependencies. (tuist#3043)
  Fix manifest loading when using Swift 5.5 (tuist#3062)
  Update project.md (tuist#3063)
  Add release name
  Bundle libProjectAutomation.dylib
  Support for tvOS Top Shelf Extensions (tuist#2793)
  Fix tuist and tuistenv release mismatch
  Version 1.44.0
  Bump version to 1.44.0
  Bump gatsby-plugin-theme-ui from 0.8.4 to 0.9.1 in /projects/website (tuist#3046)
  Bump autoprefixer from 10.2.5 to 10.2.6 in /projects/website
  Bump gatsby-plugin-sharp from 3.5.0 to 3.6.0 in /projects/website
  Bump react-spring from 9.1.2 to 9.2.1 in /projects/website
  Bump gatsby-redirect-from from 0.3.0 to 0.4.1 in /projects/website
  Bump @theme-ui/color from 0.8.4 to 0.9.1 in /projects/website
  Bump tailwindcss from 2.1.2 to 2.1.4 in /projects/website
  Bump ws from 6.2.1 to 6.2.2 in /projects/docs
  Remove stickers and shop link
  ...

# Conflicts:
#	CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pepicrft pepicrft pepicrft left review comments

@fortmarek fortmarek fortmarek approved these changes

@kwridan kwridan kwridan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tvOS Top Shelf Extension Target not supported
4 participants
@rmnblm @fortmarek @pepicrft @kwridan