Allow setting deployment target devices #541

mollyIV · 2019-10-09T18:08:29Z

Resolves #526

Short description 📝

This PR is introducing a possibility to declare deployment target in project manifest.

Solution 📦

Added deploymentTarget parameter to project manifest.

Implementation 👩‍💻👨‍💻

Create DeploymentTarget descriptor
Add DeploymentTarget to TuistGenerator.Target
Update TARGETED_DEVICE_FAMILY and *_DEPLOYMENT_TARGET in Build Settings
Update Changelog

tuistbot · 2019-10-09T18:10:01Z

	1 Warning
⚠️	Have you introduced any user-facing changes? If so, please take some time to update the documentation. Keeping the documentation up to date makes it easier for users to learn how to use Tuist.

Generated by 🚫 Danger

mollyIV · 2019-10-09T18:39:36Z

Hey @pepibumur 👋

Just started a draft PR. I added a DeploymentTarget to a Target description.

Now I am trying to apply these changes into a target generation (pbxproj under the hood). According to the documentation:

TuistGenerator: It contains all the business logic to read the project definition and generate Xcode projects

so I believe this is a folder I should take a look at. Still figuring out the core concepts of xcodeproj generation. I believe the next step should be adding deploymentTarget to TuistGenerator.Models.Target, right? 🤔 Is there any Target's property that I can have a look at the reference and do it in a similar manner?

I believe we should do the changes in DefaultSettingsProvider. We use BuildSettingsProvider.targetDefault to get target values. How we could update the one we are interested in, i.e. TARGETED_DEVICE_FAMILY?

Could you please confirm I am heading a good direction? 🙏 Thanks!

codecov · 2019-10-09T18:43:09Z

Codecov Report

Merging #541 into master will decrease coverage by 0.33%.
The diff coverage is 98.79%.

@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
- Coverage    91.6%   91.27%   -0.34%     
==========================================
  Files         370      369       -1     
  Lines       20165    19681     -484     
==========================================
- Hits        18472    17963     -509     
- Misses       1693     1718      +25

Impacted Files	Coverage Δ
Sources/ProjectDescription/Target.swift	`100% <100%> (ø)`	⬆️
Sources/TuistGenerator/Linter/SettingsLinter.swift	`100% <100%> (ø)`	⬆️
Sources/TuistGenerator/Linter/TargetLinter.swift	`97.05% <100%> (+0.47%)`	⬆️
Tests/ProjectDescriptionTests/TargetTests.swift	`100% <100%> (ø)`	⬆️
...TuistGeneratorTests/Linter/TargetLinterTests.swift	`100% <100%> (ø)`	⬆️
...neratorTests/Models/TestData/Target+TestData.swift	`100% <100%> (ø)`	⬆️
Sources/ProjectDescription/DeploymentTarget.swift	`100% <100%> (ø)`	⬆️
Sources/ProjectDescription/DeploymentDevice.swift	`100% <100%> (ø)`	⬆️
...rojectDescriptionTests/DeploymentTargetTests.swift	`100% <100%> (ø)`	⬆️
...urces/TuistGenerator/Models/DeploymentDevice.swift	`100% <100%> (ø)`	⬆️
... and 129 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 166272b...86d1980. Read the comment docs.

pepicrft · 2019-10-10T08:44:57Z

so I believe this is a folder I should take a look at. Still figuring out the core concepts of xcodeproj generation. I believe the next step should be adding deploymentTarget to TuistGenerator.Models.Target, right? 🤔 Is there any Target's property that I can have a look at the reference and do it in a similar manner?

You are right. On one side we have the models in the ProjectDescription framework that are the ones that the users use in their Project.swift files, and on the other side we have the models in the TuistGenerator that are akin to the ones in ProjectDescription but in some cases extended. For example, some paths in the ProjectDescription framework are provided as globs, and in the TuistGenerator are expanded to an array of paths. This class contains some extensions that are responsible for the mapping between the models in the two domains.

I believe we should do the changes in DefaultSettingsProvider. We use BuildSettingsProvider.targetDefault to get target values. How we could update the one we are interested in, i.e. TARGETED_DEVICE_FAMILY?

The DefaultSettingsProvider is a utility class that returns project and target build that are default to the target's product and platform. Since this is information provided by the user, I don't think that utility class is a good place to define those.

If you look at the ConfigGenerator utility class, there are some methods to generate the build settings for the project and the target. Notice that the settings are the result of extend the settings in the following order of priority:

Default settings
Base settings
Variant settings

In your case, I'd add another type of settings:

Default settings
Manifest settings
Base settings
Variant settings

That way we make the manifest settings overridable through base & variant settings. Does it make sense?

pepicrft · 2019-10-10T08:45:15Z

Sources/ProjectDescription/DeploymentTarget.swift

+public enum DeploymentTarget: Codable {
+    case iOS(String, [DeploymentDevice])
+    case macOS(String)
+    // TODO: 🙈 Add `watchOS` and `tvOS` support


Actually, what platforms should we support in this PR? 🤔

Should I leave this TODO or is it possible to support watchOS and tvOS right now?

pepicrft · 2019-10-10T08:45:46Z

The PR is looking good so far @mollyIV

Sources/TuistGenerator/Generator/ConfigGenerator.swift

mollyIV · 2019-10-10T17:45:49Z

That way we make the manifest settings overridable through base & variant settings. Does it make sense?

Yes, it does @pepibumur thank you! 🙇
I added the new changes to the PR. Would you mind to have a look? 🙏

As the next step, we need to update Build Settings to set a *_DEPLOYMENT_TARGET option, i.e.:

IPHONEOS_DEPLOYMENT_TARGET
MACOSX_DEPLOYMENT_TARGET
TVOS_DEPLOYMENT_TARGET
WATCHOS_DEPLOYMENT_TARGET

Any tip where to look for it in the codebase? 🙇

kwridan · 2019-10-10T21:27:02Z

Nice work @mollyIV! 👍

Any tip where to look for it in the codebase? 🙇

I don't believe they are currently set anywhere, perhaps set them in
ConfigGenerator.updateTargetDerived(buildSettings:...)? That currently does something similar where it sets build settings based on the various Target properties

Sources/TuistGenerator/Generator/ConfigGenerator.swift

…ompatible

Sources/TuistGenerator/Models/DeploymentTarget.swift

CHANGELOG.md

mollyIV · 2019-10-11T19:25:28Z

👋 I think this PR is ready for a round of manual testing 🎊

Could you please briefly describe the best way to test my changes manually? I can imagine the following testing steps:

Create a new project via

tuist init --name demoProject --platform ios

Update Project.swift

let project = Project(name: "App",
                      targets: [
                        Target(name: "App",
                                    platform: .iOS,
                                    deploymentTarget: .iOS("13.0", [.iphone])

Generate Xcode project via

tuist generate

Check if the deployment target was set correctly in Xcode project.

Unfortunately I am not sure how to execute tuist commands that include the changes from this PR.

Thoughts? 🙇

Sources/TuistGenerator/Linter/TargetLinter.swift

pepicrft · 2019-10-12T17:32:56Z

I left some minor comments. Besides them, I'd also:

Update any of the existing acceptance tests to use the new API. Acceptance tests are very handy to test Tuist's features end to end and detect if changes are breaking. You can read more about testing here.
I'd also add documentation for the new attribute in this file, otherwise the users won't know that this feature is available and how to use it.

After addressing this things I think we are in a good state to merge the PR. So happy to seeing this feature making it into Tuist and also your first contribution to the project 🎉

pepicrft · 2019-10-12T17:35:10Z

I forgot to add one thing. What about extending the init command to include that attribute in the generated manifest?

mollyIV · 2019-10-14T18:41:20Z

I forgot to add one thing. What about extending the init command to include that attribute in the generated manifest?

I had a look at InitCommand and I am wondering how we would like to get a deployment target from a user:

$ tuist init --name Demo --platform ios --deployment-target-version ios --deployment-target-devices iphone ipad

Thoughts? 🤔

mollyIV · 2019-10-14T18:42:10Z

I noticed all of the deploy/* jobs started failing, do you know why? 🤔

pepicrft · 2019-10-15T13:09:25Z

I had a look at InitCommand and I am wondering how we would like to get a deployment target from a user:

I'd not add more parameters. Instead, I'd default the platform most common's destination device and latest OS device.

I noticed all of the deploy/* jobs started failing, do you know why? 🤔

You can ignore those. I have to fix it 😬

mollyIV · 2019-10-15T14:37:32Z

I forgot to add one thing. What about extending the init command to include that attribute in the generated manifest?

I'd not add more parameters. Instead, I'd default the platform most common's destination device and latest OS device.

This PR contains quite a few changes already. Maybe we could create a separate task for this one and tackle it when this PR is merged?

pepicrft · 2019-10-15T16:23:27Z

This PR contains quite a few changes already. Maybe we could create a separate task for this one and tackle it when this PR is merged?

You can make the call here :). My only concern is that task might end up forgotten in the backlog.

…rget

mollyIV · 2019-10-15T19:26:37Z

Sources/TuistGenerator/Generator/ConfigGenerator.swift

@@ -191,5 +191,24 @@ final class ConfigGenerator: ConfigGenerating {
                }
            }
        }
+
+        if let deploymentTarget = target.deploymentTarget {


Any help how we could test it? Shall we add checking the settings build to an existing method in ConfigGeneratorTest or maybe create a new one?

Sure, here's an example of one of the test cases that could be written in ConfigGeneratorTests

func test_generateTargetWithDeploymentTarget() throws { // Given let target = Target.test(deploymentTarget: .iOS("12.0", [.iphone, .ipad])) // When try subject.generateTargetConfig(target, pbxTarget: pbxTarget, pbxproj: pbxproj, projectSettings: .default, fileElements: ProjectFileElements(), graph: Graph.test(), sourceRootPath: AbsolutePath("/project")) // Then let configurationList = pbxTarget.buildConfigurationList let debugConfig = configurationList?.configuration(name: "Debug") let releaseConfig = configurationList?.configuration(name: "Release") let expectedSettings = [ "TARGETED_DEVICE_FAMILY": "1,2", "IPHONEOS_DEPLOYMENT_TARGET": "12.0" ] assert(config: debugConfig, contains: expectedSettings) assert(config: releaseConfig, contains: expectedSettings) }

I added deployment target tests to ConfigGenerator, thanks for your help 🙇

mollyIV · 2019-10-15T19:28:46Z

Hello @pepibumur and @kwridan 👋

Thank you so much for your feedback and help with this PR, I really appreciate it 🙇
I did the requested changes. I'd like to kindly ask you for another round of code review 🙏 Could you also have a look at unresolved conversations and add your comments there?

I did a bit of manual testing (mostly base positive scenarios) and looks it is working as expected 💯I will be testing it more tomorrow.

Cheers 🙇

kwridan · 2019-10-15T22:47:06Z

Sources/TuistGenerator/Generator/ConfigGenerator.swift

+                settings["TARGETED_DEVICE_FAMILY"] = .string(deviceFamilyValues.map { "\($0)" }.joined(separator: ","))
+                settings["IPHONEOS_DEPLOYMENT_TARGET"] = .string(version)
+
+                if devices.contains(.ipad), devices.contains(.mac) {


ah didn't realise this ... is the iPad checkbox a pre-requisite to the Mac checkbox ? i.e. is [.iphone, .mac] an invalid option?

is the iPad checkbox a pre-requisite to the Mac checkbox ?

Exactly 👍

☝️ If the iPad is un-checked, we cannot select a Mac.

☝️ If the iPad is checked, we can select a Mac.

is [.iphone, .mac] an invalid option?

Yes, that is why we need the condition I added.

pepicrft · 2019-10-16T06:30:37Z

One more conflict to solve and I think we are good to merge this PR 🎉

pepicrft · 2019-10-16T09:37:30Z

Congrats @mollyIV on your first contribution 🎉 🎉

mollyIV added 2 commits October 9, 2019 19:52

Create DeploymentTarget descriptor

14fe176

Add DeploymentTarget to Target description

ef67e4c

Fix formatting issues

fb0252b

pepicrft reviewed Oct 10, 2019

View reviewed changes

mollyIV added 2 commits October 10, 2019 18:49

Add DeploymentTarget to TuistGenerator.Target

d1ef2ea

Update TARGETED_DEVICE_FAMILY

7486e0d

mollyIV commented Oct 10, 2019

View reviewed changes

Sources/TuistGenerator/Generator/ConfigGenerator.swift Outdated Show resolved Hide resolved

mollyIV commented Oct 10, 2019

View reviewed changes

Sources/TuistGenerator/Generator/ConfigGenerator.swift Outdated Show resolved Hide resolved

mollyIV commented Oct 10, 2019

View reviewed changes

Sources/TuistGenerator/Generator/ConfigGenerator.swift Outdated Show resolved Hide resolved

Fix formatting

cc9f231

mollyIV added 2 commits October 11, 2019 07:44

Update *_DEPLOYMENT_TARGET in Build Settings

a37720d

Fix linter issue

37ee400

mollyIV commented Oct 11, 2019

View reviewed changes

Sources/TuistGenerator/Generator/ConfigGenerator.swift Outdated Show resolved Hide resolved

mollyIV added 3 commits October 11, 2019 19:53

Make sure platform and deploymentTarget manifest parameters are c…

4e851d3

…ompatible

Move updating a deployment target to updateTargetDervied method

0e2086d

Validate os version of deployment target

c9de8b3

mollyIV commented Oct 11, 2019

View reviewed changes

Sources/TuistGenerator/Models/DeploymentTarget.swift Show resolved Hide resolved

Update Changelog

8f5ecc2

mollyIV commented Oct 11, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Add tests to generator models

5fc3300

mollyIV marked this pull request as ready for review October 12, 2019 09:31

pepicrft reviewed Oct 12, 2019

View reviewed changes

Sources/TuistGenerator/Linter/TargetLinter.swift Outdated Show resolved Hide resolved

pepicrft reviewed Oct 12, 2019

View reviewed changes

Sources/TuistGenerator/Linter/TargetLinter.swift Outdated Show resolved Hide resolved

mollyIV added 8 commits October 12, 2019 21:23

Mark a breaking change in a changelog entry

440fea8

Move a regex utility function to TuistCore

9aabecc

Make changes in public API of deployment target

bcab6cb

Fix unit tests of string regex

93909c0

Expose version property on deployment target

f400cc1

Update Target documenatation

2221d1e

Update acceptance tests

8f2c178

Fix formatting

7d3c7a0

mollyIV added 3 commits October 15, 2019 21:16

Support mac device for iOS deployment target

83c1faf

Fix lint formatting

30e915a

Enable catalyst support only if ipad is enabled for iOS deployment ta…

d9e82b7

…rget

mollyIV commented Oct 15, 2019

View reviewed changes

kwridan reviewed Oct 15, 2019

View reviewed changes

Add deployment target tests to ConfigGenerator

f408af8

Merge branch 'master' into allow-setting-deployment-target-devices

86d1980

pepicrft approved these changes Oct 16, 2019

View reviewed changes

pepicrft merged commit 2e04f8c into tuist:master Oct 16, 2019

mollyIV deleted the allow-setting-deployment-target-devices branch October 16, 2019 10:27

mollyIV mentioned this pull request Oct 17, 2019

Extend init command to include the deploymentTarget in the generated manifest #554

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting deployment target devices #541

Allow setting deployment target devices #541

mollyIV commented Oct 9, 2019 •

edited

tuistbot commented Oct 9, 2019 •

edited

mollyIV commented Oct 9, 2019 •

edited

codecov bot commented Oct 9, 2019 •

edited

pepicrft commented Oct 10, 2019

pepicrft Oct 10, 2019

mollyIV Oct 11, 2019

mollyIV Oct 16, 2019

pepicrft commented Oct 10, 2019

mollyIV commented Oct 10, 2019

kwridan commented Oct 10, 2019

mollyIV commented Oct 11, 2019

pepicrft commented Oct 12, 2019

pepicrft commented Oct 12, 2019

mollyIV commented Oct 14, 2019

mollyIV commented Oct 14, 2019

pepicrft commented Oct 15, 2019

mollyIV commented Oct 15, 2019

pepicrft commented Oct 15, 2019

mollyIV Oct 15, 2019

kwridan Oct 15, 2019

mollyIV Oct 16, 2019

mollyIV commented Oct 15, 2019

kwridan Oct 15, 2019

mollyIV Oct 16, 2019

pepicrft commented Oct 16, 2019

pepicrft commented Oct 16, 2019

Allow setting deployment target devices #541

Allow setting deployment target devices #541

Conversation

mollyIV commented Oct 9, 2019 • edited

Short description 📝

Solution 📦

Implementation 👩‍💻👨‍💻

tuistbot commented Oct 9, 2019 • edited

mollyIV commented Oct 9, 2019 • edited

codecov bot commented Oct 9, 2019 • edited

Codecov Report

pepicrft commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepicrft commented Oct 10, 2019

mollyIV commented Oct 10, 2019

kwridan commented Oct 10, 2019

mollyIV commented Oct 11, 2019

pepicrft commented Oct 12, 2019

pepicrft commented Oct 12, 2019

mollyIV commented Oct 14, 2019

mollyIV commented Oct 14, 2019

pepicrft commented Oct 15, 2019

mollyIV commented Oct 15, 2019

pepicrft commented Oct 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mollyIV commented Oct 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepicrft commented Oct 16, 2019

pepicrft commented Oct 16, 2019

mollyIV commented Oct 9, 2019 •

edited

tuistbot commented Oct 9, 2019 •

edited

mollyIV commented Oct 9, 2019 •

edited

codecov bot commented Oct 9, 2019 •

edited