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 linting for mismatching build configurations in a workspace #474

Merged

Conversation

kwridan
Copy link
Collaborator

@kwridan kwridan commented Aug 3, 2019

Resolves #467

Short description 📝

It can be easy to miss specifying a project build configuration in large workspaces. This can lead to build errors or worse an incorrectly configured built project!

Solution 📦

Add a linter to flag any missing or mismatching build configurations.

Implementation 👩‍💻👨‍💻

  • All projects' build configurations are now validated against the top level projects
  • In the event any are missing or are mismatching (e.g. have a mismatching variant) the linter will now flag them
  • Dependencies with more configurations defined won't get flagged (as those lower level dependencies can be shared in other workspaces which could have different configurations)

Example:

$ tuist generate
The following issues have been found:
  - The project 'Framework2' has missing or mismatching configurations. It has [Debug (debug), Release (release)], other projects have  [Beta (release), Debug (debug), Release (release)].
  • Add lint step
  • Update change log

Test Plan 🛠

  • Run tuist generate within fixtures/ios_app_with_multi_configs
  • Comment out any of the build configurations defined in App, Framework1 or Framework2
  • Re-run tuist generate
  • Verify a lint warning is displayed

Resolves tuist#467

- All projects' build configurations are now validated against the top level projects
- In the event any are missing or are mismatching (e.g. have a mismatching variant) the linter will now flag them

Examples:

```
$ tuist generate
The following issues have been found:
  - The project 'Framework2' has missing or mismatching configurations. It has [Debug (debug), Release (release)], other projects have  [Beta (release), Debug (debug), Release (release)].
```

Test Plan:

- Run `tuist generate` within `fixtures/ios_app_with_multi_configs`
- Comment out any of the build configurations defined in `App`, `Framework1` or `Framework2`
- Re-run `tuist generate`
- Verify a lint warning is displayed
@tuistbot
Copy link
Contributor

tuistbot commented Aug 3, 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.

Generated by 🚫 Danger

Copy link
Collaborator

@ollieatkinson ollieatkinson left a comment

Choose a reason for hiding this comment

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

Changes look good and very easy to follow. It will certainly help reduce the number of problems from defining multiple configurations! 🥇

// LintableTarget(platform: .iOS, product: .messagesExtension),
// LintableTarget(platform: .iOS, product: .stickerPack),
// LintableTarget(platform: .watchOS, product: .watch2App),
// LintableTarget(platform: .watchOS, product: .watchApp),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just double check you're happy with this new alignment, I think by formatting the file it's indented them by accident.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it was swiftformat - will push a revert, hopefully won't get flagged as an unformatted PR 🤐

Copy link
Collaborator Author

@kwridan kwridan Aug 3, 2019

Choose a reason for hiding this comment

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

ouch, will have to re-apply those changes

Screen Shot 2019-08-03 at 11 55 38 AM


func test_lint_doesNotFlagDependenciesWithExtraConfigurations() throws {
// Lower level dependencies could be shared by projects in different workspaces as such
// it is ok for them to contain more configurations than the entry node projects
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@codecov
Copy link

codecov bot commented Aug 3, 2019

Codecov Report

Merging #474 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #474      +/-   ##
==========================================
+ Coverage   92.23%   92.29%   +0.06%     
==========================================
  Files         326      326              
  Lines       16943    17076     +133     
==========================================
+ Hits        15628    15761     +133     
  Misses       1315     1315
Impacted Files Coverage Δ
...ces/TuistGenerator/Models/BuildConfiguration.swift 100% <100%> (ø) ⬆️
.../TuistGeneratorTests/Linter/GraphLinterTests.swift 100% <100%> (ø) ⬆️
...GeneratorTests/Graph/TestData/Graph+TestData.swift 98.3% <100%> (+0.09%) ⬆️
Sources/TuistGenerator/Linter/GraphLinter.swift 97.34% <100%> (+0.53%) ⬆️

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 6739265...27df26a. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Aug 3, 2019

Codecov Report

Merging #474 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #474      +/-   ##
==========================================
+ Coverage   92.23%   92.29%   +0.06%     
==========================================
  Files         326      326              
  Lines       16943    17076     +133     
==========================================
+ Hits        15628    15761     +133     
  Misses       1315     1315
Impacted Files Coverage Δ
...ces/TuistGenerator/Models/BuildConfiguration.swift 100% <100%> (ø) ⬆️
.../TuistGeneratorTests/Linter/GraphLinterTests.swift 100% <100%> (ø) ⬆️
...GeneratorTests/Graph/TestData/Graph+TestData.swift 98.3% <100%> (+0.09%) ⬆️
Sources/TuistGenerator/Linter/GraphLinter.swift 97.34% <100%> (+0.53%) ⬆️

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 6739265...27df26a. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 3, 2019

Codecov Report

Merging #474 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #474      +/-   ##
==========================================
+ Coverage   92.23%   92.29%   +0.06%     
==========================================
  Files         326      326              
  Lines       16943    17076     +133     
==========================================
+ Hits        15628    15761     +133     
  Misses       1315     1315
Impacted Files Coverage Δ
...ces/TuistGenerator/Models/BuildConfiguration.swift 100% <100%> (ø) ⬆️
.../TuistGeneratorTests/Linter/GraphLinterTests.swift 100% <100%> (ø) ⬆️
...GeneratorTests/Graph/TestData/Graph+TestData.swift 98.3% <100%> (+0.09%) ⬆️
Sources/TuistGenerator/Linter/GraphLinter.swift 97.34% <100%> (+0.53%) ⬆️

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 6739265...739ca47. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 3, 2019

Codecov Report

Merging #474 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #474      +/-   ##
==========================================
+ Coverage   92.23%   92.29%   +0.06%     
==========================================
  Files         326      326              
  Lines       16943    17076     +133     
==========================================
+ Hits        15628    15761     +133     
  Misses       1315     1315
Impacted Files Coverage Δ
...ces/TuistGenerator/Models/BuildConfiguration.swift 100% <100%> (ø) ⬆️
.../TuistGeneratorTests/Linter/GraphLinterTests.swift 100% <100%> (ø) ⬆️
...GeneratorTests/Graph/TestData/Graph+TestData.swift 98.3% <100%> (+0.09%) ⬆️
Sources/TuistGenerator/Linter/GraphLinter.swift 97.34% <100%> (+0.53%) ⬆️

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 6739265...27df26a. Read the comment docs.

private func lintMismatchingConfigurations(graph: Graphing) -> [LintingIssue] {
let entryNodeProjects = graph.entryNodes.compactMap { $0 as? TargetNode }.map { $0.project }

let knownConfgiruations = entryNodeProjects.reduce(into: Set()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: knownConfgiruations

@kwridan kwridan merged commit a3684c6 into tuist:master Aug 4, 2019
@kwridan kwridan deleted the features/lint-mismatching-configurations branch August 4, 2019 09:02
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.

Lint projects graph to ensure matching configurations are used in all projects
4 participants