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 compatible Xcodes option to the TuistConfig #476

Merged
merged 12 commits into from Aug 7, 2019
Merged

Conversation

pepicrft
Copy link
Contributor

@pepicrft pepicrft commented Aug 4, 2019

Short description 📝

Xcode doesn't support binding projects to a specific version of Xcode. Since new versions of Xcode come with new versions of Swift, which often break projects, it might be useful for teams to have a way that prevents developers in the team from using the projects with a version of Xcode that is not compatible with the project in hands.

Solution 📦

Add a new setting to the TuistConfig object:

let config = TuistConfig(compatibleXcodeVersions: ["10.3"])

When a project gets generated, Tuist runs a validation against the Xcode version selected in the system. If the project is not compatible, Tuist yields the following output:

incompatible-xcode

Implementation 👩‍💻👨‍💻

  • Add XcodeController to TuistCore.
  • Add CompatibleXcodeVersions to TuistGenerator and ProjectDescription.
  • Add TuistConfigLinter and run it as part of the project generation.
  • Unit tests all the changes.
  • Add an acceptance test that fails because the Xcode version is not compatible.

@pepicrft pepicrft requested a review from a team August 4, 2019 10:50
@pepicrft pepicrft self-assigned this Aug 4, 2019
@tuistbot
Copy link
Contributor

tuistbot commented Aug 4, 2019

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.

SwiftLint found issues

Warnings

File Line Reason
XCTestCase+Extras.swift 16 Tuples should have at most 2 members. (large_tuple)
TuistConfig.swift 38 Line should be 150 characters or less: currently 156 characters (line_length)

Rubocop violations

File Line Reason
features/step_definitions/shared/tuist.rb 18 Useless assignment to variable - expected_msg. (https://shopify.github.io/ruby-style-guide/#underscore-unused-vars)

Generated by 🚫 Danger

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 @pepibumur!

I think it's valuable to have the notion of compatible Xcode versions for the generation process, especially as we start leveraging more Xcode 11 features soon! Features like #394 may require Tuist to warn users that their current version Xcode doesn't support certain features that will be in the generated project.

On a slightly philosophical note, I wonder if the use case here may be more akin to environment validation (environmentLinter.lint(using: config) which could deserve its own command as opposed to being part of the project generation process. I can see the values both ways, I guess it's something for us to keep in mind.

Tests/TuistCoreTests/Xcode/XcodeControllerTests.swift Outdated Show resolved Hide resolved
Sources/TuistCore/Xcode/XcodeController.swift Show resolved Hide resolved
fixtures/ios_app_with_incompatible_xcode/TuistConfig.swift Outdated Show resolved Hide resolved
Sources/TuistGenerator/Generator/Generator.swift Outdated Show resolved Hide resolved
Sources/TuistCore/Utils/Printer.swift Outdated Show resolved Hide resolved
@pepicrft
Copy link
Contributor Author

pepicrft commented Aug 7, 2019

On a slightly philosophical note, I wonder if the use case here may be more akin to environment validation (environmentLinter.lint(using: config) which could deserve its own command as opposed to being part of the project generation process. I can see the values both ways, I guess it's something for us to keep in mind.

This is a really good point. I'm inclined to rename TuistConfigLinter to EnvironmentLinter because that's what we are doing in the end. I like the idea of having a separate command, but I'd default to running it as part of the project generation. I think we should be strict with not generating the project if we know it's not going to compile.

@pepicrft
Copy link
Contributor Author

pepicrft commented Aug 7, 2019

✅ Renamed TuistConfigLinter to EnvironmentLinter

@codecov
Copy link

codecov bot commented Aug 7, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@1aed58f). Click here to learn what that means.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #476   +/-   ##
=========================================
  Coverage          ?   92.41%           
=========================================
  Files             ?      348           
  Lines             ?    17803           
  Branches          ?        0           
=========================================
  Hits              ?    16452           
  Misses            ?     1351           
  Partials          ?        0
Impacted Files Coverage Δ
...IntegrationTests/Generator/Mocks/MockPrinter.swift 30% <0%> (ø)
...uistGenerator/Models/CompatibleXcodeVersions.swift 0% <0%> (ø)
...GeneratorTests/Linter/EnvironmentLinterTests.swift 100% <100%> (ø)
Sources/TuistGenerator/Generator/Generator.swift 100% <100%> (ø)
Sources/TuistCoreTesting/Utils/MockPrinter.swift 73.17% <100%> (ø)
...ests/TuistCoreTests/Linter/LintingIssueTests.swift 100% <100%> (ø)
...uistCoreTesting/Extensions/XCTestCase+Extras.swift 89.65% <100%> (ø)
Sources/TuistCore/Xcode/Xcode.swift 100% <100%> (ø)
...rator/MultipleConfigurationsIntegrationTests.swift 96.54% <100%> (ø)
...istCoreTesting/Xcode/TestData/Xcode+TestData.swift 100% <100%> (ø)
... and 17 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 1aed58f...b3a3334. Read the comment docs.

@pepicrft pepicrft merged commit 2b09f5c into master Aug 7, 2019
@pepicrft pepicrft deleted the required-xcode branch August 7, 2019 17:44
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.

None yet

3 participants