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

Fix #253 - Include linked library and embed in any top level executable bundle #259

Merged
merged 7 commits into from
Mar 7, 2019

Conversation

ollieatkinson
Copy link
Collaborator

@ollieatkinson ollieatkinson commented Mar 7, 2019

The P/R introduces two new checks to Graph:

  1. When generating the linkable dependencies, include any transitive frameworks.
  2. When generating the embeddable frameworks, include any transitive frameworks.

I added a test fixture, @enhorn and @Rag0n can you please check if this works with your use-case.

@ollieatkinson ollieatkinson requested a review from a team March 7, 2019 15:03
@Rag0n
Copy link
Contributor

Rag0n commented Mar 7, 2019

It doesn't work:

Embedding framework ../../Carthage/Build/iOS/RxSwift.framework
❌ Error: The build variable CONFIGURATION is missing.
❌ Error: Command exited with error code 1 and error: 
Command PhaseScriptExecution failed with a nonzero exit code

Also I'm not sure, but probably we should not link against transitive frameworks.

@ollieatkinson
Copy link
Collaborator Author

ollieatkinson commented Mar 7, 2019

@Rag0n how are you testing this version?

Re the linking - you're right it doesn't make sense. I will remove.

@Rag0n
Copy link
Contributor

Rag0n commented Mar 7, 2019

@Rag0n how are you testing this version?

I dont know how to test it in the right way, my flow is:
checkout -> make package-release -> copy build into .tuist-bin

@tuistbot
Copy link
Contributor

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

Rubocop violations

File Line Reason
features/step_definitions/generate.rb 1 Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
features/step_definitions/generate.rb 1 Metrics/LineLength: Line is too long. [164/120] (https://shopify.github.io/ruby-style-guide/#80-character-limits)
features/step_definitions/generate.rb 14 Metrics/LineLength: Line is too long. [167/120] (https://shopify.github.io/ruby-style-guide/#80-character-limits)
features/step_definitions/generate.rb 25 Layout/TrailingBlankLines: Final newline missing. (https://shopify.github.io/ruby-style-guide/#newline-eof)
features/step_definitions/shared/workspace.rb 10 Lint/UnusedBlockArgument: Unused block argument - scenario. You can omit the argument if you don't care about it. (https://shopify.github.io/ruby-style-guide/#underscore-unused-vars)
features/support/xcode.rb 1 Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
features/support/xcode.rb 4 Layout/SpaceInsideBlockBraces: Space between { and
features/support/xcode.rb 4 Layout/SpaceInsideBlockBraces: Space missing inside }.
features/support/xcode.rb 9 Layout/TrailingWhitespace: Trailing whitespace detected. (https://shopify.github.io/ruby-style-guide/#no-trailing-whitespace)
features/support/xcode.rb 10 Layout/TrailingWhitespace: Trailing whitespace detected. (https://shopify.github.io/ruby-style-guide/#no-trailing-whitespace)
features/support/xcode.rb 28 Layout/TrailingBlankLines: Final newline missing. (https://shopify.github.io/ruby-style-guide/#newline-eof)

Generated by 🚫 Danger

@ollieatkinson
Copy link
Collaborator Author

ollieatkinson commented Mar 7, 2019

@Rag0n - seems the tuistenv command was swallowing up the environment variables. I pushed up the new tuistenv command to this branch which fixes it.

  1. make package-release
  2. copy build into .tuist-bin
  3. tuist generate
  4. update the build phase for "Embed Precompiled Frameworks" to reference the new tuistenv command e.g.
/Users/oat01/Checkouts/tuist/build/tuistenv embed ../Framework2/Framework2.framework

this will be automatically done with the new release.

@Rag0n
Copy link
Contributor

Rag0n commented Mar 7, 2019

It works. Good job, thank you!

Copy link
Collaborator Author

@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.

🍨

features/step_definitions/shared/workspace.rb Outdated Show resolved Hide resolved
features/support/xcode.rb Outdated Show resolved Hide resolved
features/step_definitions/generate.rb Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #259 into master will increase coverage by 0.07%.
The diff coverage is 98.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #259      +/-   ##
==========================================
+ Coverage   87.65%   87.72%   +0.07%     
==========================================
  Files         255      257       +2     
  Lines        9049     9102      +53     
==========================================
+ Hits         7932     7985      +53     
  Misses       1117     1117
Impacted Files Coverage Δ
Sources/TuistKit/Graph/Graph.swift 90.78% <100%> (+1.2%) ⬆️
Sources/TuistKit/Generator/LinkGenerator.swift 90.43% <100%> (+0.25%) ⬆️
Sources/TuistEnvKit/Commands/CommandRunner.swift 98.41% <100%> (ø) ⬆️
Tests/TuistKitTests/Graph/GraphTests.swift 100% <100%> (ø) ⬆️
Sources/TuistCore/Extensions/Sequence+Filter.swift 100% <100%> (ø)
Sources/TuistKit/Utils/BinaryLocator.swift 85.71% <85.71%> (ø)

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 99537d0...42a0c28. Read the comment docs.

@pepicrft pepicrft merged commit de54857 into master Mar 7, 2019
@pepicrft pepicrft deleted the issue-253 branch March 7, 2019 23:31
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.

4 participants