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 platformFilters attribute #737

Merged
merged 3 commits into from
Dec 21, 2022

Conversation

maxwellE
Copy link
Contributor

@maxwellE maxwellE commented Dec 8, 2022

Adds this attribute to PBXBuildFile and
PBXTargetDependency

Adds this attribute to PBXBuildFile and
PBXTargetDependency
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.

thanks for adding this.

A small suggestion for future contributions, it would be really helpful to capture where / when some of the attributes appear to aid testing and verifying the behaviours.

Luckily I was able to figure out where to look for this :)

In Xcode the filters option appears for targets and allows selecting all, single or multiple platforms

Filters

It appears selecting an individual platform filter results in the attribute platformFilter being used:

single-filter

3DFCA495EE0D3913DAD9804B /* Framework2.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = B7F0EC5C1D588EC72A1C5AEA /* Framework2.framework */; platformFilter = ios; };

vs selecting multiple platforms, results in the attribute platformFilters (plural) being used:

multiple-filters

3DFCA495EE0D3913DAD9804B /* Framework2.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = B7F0EC5C1D588EC72A1C5AEA /* Framework2.framework */; platformFilters = (ios, macos, tvos, ); };

From what I gather those are mutually exclusive. Xcode seems to prioritise platformFilters if both are present however upon making any changes would get rid on platformFilter. To maintain a backwards compatible API the approach taken here looks reasonable, but perhaps for a future major version we could consider consolidating the properties and internally have XcodeProj use the appropriate attribute based on single vs multiple filters.

@brentleyjones
Copy link
Contributor

Just a counter-point, Xcode sometimes sets single element versions of the plural value:

platformFilters = (tvos, );

@maxwellE
Copy link
Contributor Author

thanks for adding this.

A small suggestion for future contributions, it would be really helpful to capture where / when some of the attributes appear to aid testing and verifying the behaviours.

Luckily I was able to figure out where to look for this :)

In Xcode the filters option appears for targets and allows selecting all, single or multiple platforms

Filters

It appears selecting an individual platform filter results in the attribute platformFilter being used:

single-filter

3DFCA495EE0D3913DAD9804B /* Framework2.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = B7F0EC5C1D588EC72A1C5AEA /* Framework2.framework */; platformFilter = ios; };

vs selecting multiple platforms, results in the attribute platformFilters (plural) being used:

multiple-filters

3DFCA495EE0D3913DAD9804B /* Framework2.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = B7F0EC5C1D588EC72A1C5AEA /* Framework2.framework */; platformFilters = (ios, macos, tvos, ); };

From what I gather those are mutually exclusive. Xcode seems to prioritise platformFilters if both are present however upon making any changes would get rid on platformFilter. To maintain a backwards compatible API the approach taken here looks reasonable, but perhaps for a future major version we could consider consolidating the properties and internally have XcodeProj use the appropriate attribute based on single vs multiple filters.

I will make sure to follow this workflow next time.

@kwridan
Copy link
Collaborator

kwridan commented Dec 21, 2022

Just a counter-point, Xcode sometimes sets single element versions of the plural value:

fun times 😅

I will make sure to follow this workflow next time.

Thank you

@kwridan kwridan merged commit 6e96bdc into tuist:main Dec 21, 2022
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.

3 participants