Implement enable/disable-get-task-allow for swift build (#8378) #9380
Conversation
|
With this code |
There was a problem hiding this comment.
Thanks for the PR! This change looks like a good improvement to me, but I'd like to double check a few details and give other folks a chance to review as well, which might take a few days. In the meantime I left a handful of minor comments.
With this code BuildCommandsTestCase.doesNotRebuildWithFlags(data:flags:) test fails for swift build backend and debug configuration. I think it’s because even though swift build does not rebuild the binary, it is still modified by codesign process executed by swift build. I guess easy “fix” could be passing —disable-get-task-allow-entitlement in that test, but it doesn’t seem elegant/right.
I’d appreciate advice about what to do with this test.
Looks like you exposed a case where the task's signature was pulling in some irrelevant settings overrides and invalidating it too often. With your patch layered on top of swiftlang/swift-build#920, I'm now seeing the test pass
| ) async -> SWBProvisioningTaskInputs { | ||
| let identity = provisioningSourceData.signingCertificateIdentifier | ||
| // if we need to add debug entitlement we have to do codesigning, so we need to ensure at least ad-hoc signing | ||
| let identity = if provisioningSourceData.signingCertificateIdentifier |
There was a problem hiding this comment.
Instead of falling back to "-" here, I wonder if this would be a little easier to follow if we added "CODE_SIGN_IDENTITY=-" as an override in makeBuildParameters. @jakepetroules do you have any opinions?
There was a problem hiding this comment.
I think we shouldn't need to provide an override at all. On macOS, CODE_SIGN_IDENTITY defaults to ad-hoc signing if no DEVELOPMENT_TEAM is set, which is already a good default for the SwiftPM CLI.
I don't remember off the top of my head what signingCertificateIdentifier being empty means, but if we need to handle that case you could just adjust the branch below to do if identity == "-" || identity.isEmpty instead of overriding the identity.
There was a problem hiding this comment.
when using swiftbuild it seems identity is empty, hence me changing it to ad-hoc: otherwise binary is just signed ad-hoc by linker
if identity == "-" || identity.isEmpty seems simple enough so I can go with that if there is no special meaning to signingCertificateIdentifier being empty
| ) async -> SWBProvisioningTaskInputs { | ||
| let identity = provisioningSourceData.signingCertificateIdentifier | ||
| // if we need to add debug entitlement we have to do codesigning, so we need to ensure at least ad-hoc signing | ||
| let identity = if provisioningSourceData.signingCertificateIdentifier |
There was a problem hiding this comment.
I think we shouldn't need to provide an override at all. On macOS, CODE_SIGN_IDENTITY defaults to ad-hoc signing if no DEVELOPMENT_TEAM is set, which is already a good default for the SwiftPM CLI.
I don't remember off the top of my head what signingCertificateIdentifier being empty means, but if we need to handle that case you could just adjust the branch below to do if identity == "-" || identity.isEmpty instead of overriding the identity.
| try await withKnownIssue(isIntermittent: (ProcessInfo.hostOperatingSystem == .linux)) { | ||
| let buildSystem = data.buildSystem | ||
| let buildConfiguration = data.config | ||
| try await withKnownIssue(isIntermittent: ProcessInfo.hostOperatingSystem == .linux) { |
There was a problem hiding this comment.
This test does not appear to do anything on linux or Windows. if that is intended, consider removing the try await withKnownIssue and add the .requireHostOS(.macOS) (or something similar) trait to the test.
There was a problem hiding this comment.
I'll be removing withKnownIssue due to comment on line R1285, but by "does not appear to do anything" do you mean that its current behaviour on Linux and Windows is not useful enough to warrant running it?
Currently it's checking for warning message in output when using enable- or disable-get-task-allow options and ensuring no codesign call message is in output log (though build would fail on Linux or Windows if such call occurred when I think about it), which is the same test behaviour as currently on main.
I can confirm it does check for that when running it in dev container or docker image (though this test fails at first fixture build command for swiftbuild because of linker error about -pie and static being not compatible, but it doesn't look related to my changes), though I do recognize that it doesn't check for much and may not be useful test at all on Linux or Windows, so I just want to confirm that this is what you mean.
svmkr-dev
requested review from
bripeticca,
cmcgee1024,
daveyc123,
dschaefer2,
plemarquand and
rconnell9
as code owners
svmkr-dev
force-pushed
the
swift-build-get-task-allow
branch
from
4d30925 to
e281a8c
Compare
Rewrite BuildCommandTests.getTaskAllowEntitlement to check for entitlements using codesign. Add debugging entitlement during build process using swift build. Fix wording in warning related to get-task-allow command line options.
Use DEPLOYMENT_POSTPROCESSING set to NO instead of ENTITLEMENTS_DONT_REMOVE_GET_TASK_ALLOW when get-task-allow entitlement is requested to ensure release builds keep entitlement when needed.
svmkr-dev
force-pushed
the
swift-build-get-task-allow
branch
from
abf1552 to
099d761
Compare
owenv
left a comment
owenv
left a comment
There was a problem hiding this comment.
thanks! This tentatively lgtm but I'd like to review test results before approving
|
@swift-ci test |
4 participants
Implement enable/disable-get-task-allow for swift build backend
Motivation:
Resolve issue #8378
Modifications:
Rewrite of BuildCommandTest.getTaskAllowedEntitlement to check for entitlements using
codesign. Also use build type as test parameter to shorten test code slightly.In
SwiftBuildSystemPlanningOperationsDelegatereceives argument used to decide whether add macOSget-task-allowentitlement when Swift Build requests provisioning data. When we want to add entitlement we need to use at least ad-hoc signing so code also ensures it is done. It is only added on macOS.Added settings override for swift-build instructing to not remove get-task-allow when we want to have it.
Also fixed wording in warning produced when user uses get-task-allow command line options on unsupported platforms.
Result:
When building with swift-build backend:
--enable-get-task-allow-entitlementwill have macOS get-task-allow entitlement