Fix platform filter handling for binary target build files - #1600
Conversation
|
@swift-ci test |
owenv
left a comment
There was a problem hiding this comment.
Thanks. This looks good to me, but I think we should future proof build configuration filters too to avoid a similar regression in the future
| ) | ||
| case .reference, .namedReference: | ||
| // Binary targets do not support build configuration filters. | ||
| assert(aggregatedBuildConfigurationFilters.isEmpty) |
There was a problem hiding this comment.
The intent is that eventually binary targets could condition on build configuration as well, so I think we should drop this assert at the same time to future proof things.
There was a problem hiding this comment.
Just to clarify, do you mean that I should only remove the assertion for now, or also apply aggregatedBuildConfigurationFilters to the returned BuildFile by extending with(...) as follows?
case .reference, .namedReference:
- // Binary targets do not support build configuration filters.
- assert(aggregatedBuildConfigurationFilters.isEmpty)
return firstBuildFile.with(
- platformFilters: aggregatedPlatformFilters
+ platformFilters: aggregatedPlatformFilters,
+ buildConfigurationFilters: aggregatedBuildConfigurationFilters
)There was a problem hiding this comment.
I think we should extend with and also use the aggregatedBuildConfigurationFilters, so the two types of filters are handled consistently
There was a problem hiding this comment.
Thank you for the feedback. I’ve updated the implementation to aggregate build configuration filters as well and updated the test accordingly.
|
@swift-ci test |
|
@yimajo By the way, if you're interested in getting CI access to start tests yourself, there's some information about that in https://www.swift.org/contributing/#contributing-code (the "Member" section) if you're not already aware. I think you probably qualify based on the PRs you've been submitting here. |
0449e51 to
43bd44c
Compare
|
@swift-ci test |
For binary targets, this change removes the incorrect
assertfor platform filters and applies the union of the platform filters to the returnedBuildFile.Motivation
When an app uses a Swift package that depends on a binary target, running Swift Build with assertions enabled fails at the following assertion:
swift-build/Sources/SWBTaskConstruction/TaskProducers/BuildPhaseTaskProducers/SwiftPackageCopyFilesTaskProducer.swift
Line 185 in 99885a7
The issue can be reproduced with the following
Package.swift:The issue can be reproduced by linking both package products from an app.
A target that depends on a binary target can specify platform conditions using
.when(platforms:). SwiftPM carries these conditions asplatformFilterson the correspondingBuildFiles.In this example, the same binary target is reached through two package products with different platform filters. Both dependency paths participate in a macOS build, so
aggregatedPlatformFiltersis nonempty and the assertion fails.Changes
Removing
assert(aggregatedPlatformFilters.isEmpty)is sufficient to prevent the assertion failure. However, returningfirstBuildFileunchanged would preserve only one set of platform filters and discard the aggregated union.This change:
assert(aggregatedPlatformFilters.isEmpty).aggregatedPlatformFiltersto the returnedBuildFilefor binary targets.assert(aggregatedBuildConfigurationFilters.isEmpty), because SwiftPM does not support conditionalizing a binary-target dependency by build configuration.BuildableItembranching as aswitch.firstBuildFileif a.targetProductcannot be resolved, while usingassertionFailureto report the violated invariant when assertions are enabled.Testing
Added
SwiftPackageCopyFilesTaskProducerTests.packageBinaryXCFrameworkPlatformFiltersAreAggregated().The test verifies that
SwiftPackageCopyFilesTaskProducer.buildFilesForPackages(context:frameworksBuildPhase:)returns aBuildFilecontaining the union of the macOS, iOS, and iOS Simulator platform filters.