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 copy files build phase comparator #40

Merged

Conversation

marciniwanicki
Copy link
Contributor

@marciniwanicki marciniwanicki commented Jan 9, 2020

Resolves: #17

Describe your changes
Added copy files build phase comparator which allow to find differences in "Extensions" or "Embed Frameworks" build phases. The comparator only compares common build phases (build phases with the same name in both projects). Name or order differences can be picked up by BuildPhasesComparator.

Testing performed

  • CI / unit and integration tests
  • Inspect command tests output
  • Copy an existing project, edit one of the copy files build phases in one of the projects, make sure all the changes are reported when running (xcdiff -g copy_files):
    • add a file
    • modify file properties
    • update properties i.e. destination path

Signed-off-by: Marcin Iwanicki <miwanicki1@bloomberg.net>
@codecov
Copy link

codecov bot commented Jan 9, 2020

Codecov Report

Merging #40 into master will increase coverage by 1.04%.
The diff coverage is 99.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   95.01%   96.05%   +1.04%     
==========================================
  Files          40       42       +2     
  Lines        1787     1878      +91     
==========================================
+ Hits         1698     1804     +106     
+ Misses         89       74      -15
Impacted Files Coverage Δ
Sources/XCDiffCore/Library/TargetsHelper.swift 93.12% <ø> (-0.25%) ⬇️
...es/XCDiffCore/Library/SettingValueComparator.swift 100% <ø> (+27.02%) ⬆️
Sources/XCDiffCore/Library/PBX+Extensions.swift 100% <100%> (ø)
...XCDiffCore/Comparator/DependenciesComparator.swift 94.02% <100%> (-0.76%) ⬇️
.../XCDiffCore/Comparator/BuildPhasesComparator.swift 100% <100%> (ø) ⬆️
Sources/XCDiffCore/Library/String+Extensions.swift 100% <100%> (ø) ⬆️
Sources/XCDiffCore/ComparatorType.swift 100% <100%> (ø) ⬆️
...es/XCDiffCore/Library/Collections+Extensions.swift 100% <100%> (+16.66%) ⬆️
...es/XCDiffCore/Comparator/CopyFilesComparator.swift 99.27% <99.27%> (ø)
... and 1 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 02c720d...e1f43bc. Read the comment docs.

…d phase, update docs

Signed-off-by: Marcin Iwanicki <miwanicki1@bloomberg.net>
Signed-off-by: Marcin Iwanicki <miwanicki1@bloomberg.net>
Signed-off-by: Marcin Iwanicki <miwanicki1@bloomberg.net>
marciniwanicki added a commit to tuist/tuist that referenced this pull request Jan 13, 2020
… phases (#886)

### Short description 📝

While working on bloomberg/xcdiff#40, we discovered that some of the build files are missing `RemoveHeadersOnCopy` attribute. The attribute determines if the headers should be striped from the archive, and if not set, can lead to headers leakage. The attribute is not visible in the Xcode UI, but added automatically every time a certain bundle types are added to a copy files build phase (i.e. .framework, .appex).
Oddly the attribute is not added for `.xcframework`, but it's probably an Xcode issue (FB7533618).

### Solution 📦

Added "RemoveHeadersOnCopy" attribute for build files in embed precompiled frameworks, embed watch content, and embed app extensions build phases.

### Implementation 👩‍💻👨‍💻

- [x] Update TuistGenerator to add `RemoveHeadersOnCopy`
- [x] Update acceptance tests
- [x] Update CHANGELOG
Copy link
Contributor

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

Just realised the "dependencies" comparator is somewhat incomplete:

e.g. Dependencies in the first build phase here aren't compared

Screenshot 2020-01-14 at 10 14 20

we can address is in a separate PR (added #41).

Comment on lines +13 to +16
### `copy_files`

Compares copy files build phases i.e. embed frameworks, extensions

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@marciniwanicki marciniwanicki merged commit a1ca6db into bloomberg:master Jan 14, 2020
@marciniwanicki marciniwanicki deleted the feature/copy-files-comparator branch January 14, 2020 16:42
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.

Add app extensions comparator
2 participants