Skip to content

remove platforms from ResolvedPackage #4206

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Mar 9, 2022

motivation: the call sites that use package level platforms are incorrect and can be removed or updated to use product level

changes:

  • remove use of package level platforms from PIF generator
  • update build plan to use product level platforms
  • adjust tests

@tomerd
Copy link
Contributor Author

tomerd commented Mar 9, 2022

@neonichu

@@ -2490,62 +2466,14 @@ class PIFBuilderTests: XCTestCase {
}
}

func testSDKOptions() throws {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@neonichu this has a dedicated test - need to make sure we can drop it

Copy link
Contributor

@abertelrud abertelrud left a comment

Choose a reason for hiding this comment

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

This looks good to me, but would want to hear @neonichu's opinion as well. The target settings override the project settings, so the important thing is that each target sets the right values, as seems to be the case. So I think the dedicated test case you reference can be removed as well.

@neonichu
Copy link
Contributor

I think we have to make more changes to PIFBuilder and its tests. We should add assertion for the various deployment targets at the target level to the tests and set both deployment targets as well as SPECIALIZATION_SDK_OPTIONS. As far as I can see, only test targets are getting a deployment target set after the changes (https://github.com/apple/swift-package-manager/blob/main/Sources/XCBuildSupport/PIFBuilder.swift#L415-L421) but no other target types.

@abertelrud
Copy link
Contributor

abertelrud commented Mar 16, 2022

I think we have to make more changes to PIFBuilder and its tests. We should add assertion for the various deployment targets at the target level to the tests and set both deployment targets as well as SPECIALIZATION_SDK_OPTIONS. As far as I can see, only test targets are getting a deployment target set after the changes (https://github.com/apple/swift-package-manager/blob/main/Sources/XCBuildSupport/PIFBuilder.swift#L415-L421) but no other target types.

Would it be worth also making sure this case is covered by an end-to-end functional test? It would have to be macOS-only since it relies on xcbuild, but doing the build and then using otool to check what deployment target ended up getting set in the binary seems like the ultimate check whether this works as intended. Otherwise the code could still be doing the "wrong thing" even if the unit test that checks the PIF contents is made to pass.

motivation: the call sites that use package level platforms are incorrect and can be removed or updated to use product level

changes:
* remove use of package level platforms from PIF generator
* update build plan to use product level platforms
* adjust tests
@tomerd tomerd force-pushed the refactor/remove-package-platforms branch from 5656dbf to 4becc8e Compare March 25, 2022 05:13
@tomerd
Copy link
Contributor Author

tomerd commented Mar 25, 2022

@swift-ci smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Mar 25, 2022

@neonichu looks like this is causing some failures on the integration tests when using --build-system = xcode. can you confirm the PIF changes here?

@neonichu neonichu added the next waiting for next merge window label Apr 11, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
next waiting for next merge window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants