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.

Sign up for GitHub

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

Jump to bottom

rdar://105991217 (Make macros testable) #6312

Merged
merged 1 commit into from
Mar 27, 2023
Merged

rdar://105991217 (Make macros testable) #6312

merged 1 commit into from
Mar 27, 2023

Conversation

neonichu
Copy link
Contributor

Opt macros into testable executables if we're building them as executables.

@neonichu neonichu self-assigned this Mar 22, 2023
@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

tomerd
tomerd approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

I would have preferred a clearer variable name ,but none blocking.

does this need a test added?

@neonichu
Copy link
Contributor Author

neonichu commented Mar 22, 2023 •
edited
Loading

macOS is failing with

/private/var/folders/pf/p5g2vb2j0yx1x6vlmxdppc700000gn/T/Miscellaneous_AtMainSupport.nnOfYt/Miscellaneous_AtMainSupport/Package.swift:1:8: error: failed to build module 'Foundation'; this SDK is not supported by the compiler (the SDK is built with 'Apple Swift version 5.5 (swiftlang-1300.0.27.6 clang-1300.0.27.2)', while this compiler is 'Apple Swift version 5.9-dev (LLVM ba3a5bcc3ba30ba, Swift 72e591975928e42)'). Please select a toolchain which matches the SDK.
import Foundation
       ^

@neonichu
Copy link
Contributor Author

I would have preferred a clearer variable name

Any suggestions? Happy to take them since I'll anyway will add a test as well.

@tomerd
Copy link
Contributor

tomerd commented Mar 23, 2023

Any suggestions? Happy to take them since ill anyway will add a test as well.

did not think about this too deeply, but my initial thought maybe this can be an extension on target isTestableMacro or something like that

@MaxDesiatov
Copy link
Contributor

+1 on changing the name to isTestableMacro

@neonichu
Copy link
Contributor Author

neonichu commented Mar 24, 2023 •
edited
Loading

I think isTestableMacro is very awkward, since most targets are not macros, so why would they have this property? A generic isTestable also doesn't work since this is special behavior only for executable-ish targets. usesTestableExecutablesFeature?

neonichu
neonichu commented Mar 24, 2023
View reviewed changes
Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift Outdated
let dirPath = (target.type == .executable && !allowLinkingAgainstExecutables) ? self.tempsPath : self
.isLinux() || self.buildParameters.triple.isWindows()) && self.toolsVersion >= .v5_5
let supportsTestableExecutablesFeature = (target as? SwiftTarget)?.supportsTestableExecutablesFeature == true
let dirPath = (supportsTestableExecutablesFeature && !allowLinkingAgainstExecutables) ? self.tempsPath : self
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this wasn't actually checking for is SwiftTarget before which means this check applied beyond the testable executables feature. I'm reverting this to be as before to avoid unnecessary changes.

@neonichu
rdar://105991217 (Make macros testable)
d632508 
Opt macros into testable executables if we're building them as executables.
@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

does this need a test added?

Not sure yet how to write a test for this, I wanted to do an integration test since that seems to be the most useful in this case, but that is not possible because it requires a SwiftSyntax package dependency which seems prohibitive to include for several reasons.

@neonichu
Copy link
Contributor Author 

======UPDATE FAILURES======
/Users/ec2-user/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/swift-certificates failed (ret=128): ['git', 'fetch', '--recurse-submodules=yes', '--tags']
fatal: unable to access '/~https://github.com/apple/swift-certificates.git/': Received HTTP code 503 from proxy after CONNECT

/Users/ec2-user/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/ninja failed (ret=128): ['git', 'fetch', '--recurse-submodules=yes', '--tags']
fatal: unable to access '/~https://github.com/ninja-build/ninja.git/': Received HTTP code 503 from proxy after CONNECT

update-checkout failed, fix errors and try again

@MaxDesiatov
Copy link
Contributor

@swift-ci smoke test macos

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@neonichu
Copy link
Contributor Author

Going to merge as-is since it isn't clear how to add a test at this time.

@neonichu neonichu merged commit e02a8b5 into main Mar 27, 2023
@neonichu neonichu deleted the testable-macros branch March 27, 2023 15:14
MaxDesiatov
MaxDesiatov reviewed Mar 28, 2023
View reviewed changes
Sources/Build/BuildPlan.swift
@@ -731,10 +731,10 @@ public class BuildPlan: SPMBuildCore.BuildPlan {
// In tool version .v5_5 or greater, we also include executable modules implemented in Swift in
// any test products... this is to allow testing of executables. Note that they are also still
// built as separate products that the test can invoke as subprocesses.
case .executable, .snippet:
case .executable, .snippet, .macro:
Copy link
Contributor

@MaxDesiatov MaxDesiatov Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This now causes a warning when building:

Build/BuildPlan.swift:770:22: warning: case is already handled by previous patterns; consider removing it
                case .macro:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxDesiatov MaxDesiatov MaxDesiatov left review comments

@tomerd tomerd tomerd approved these changes

@abertelrud abertelrud Awaiting requested review from abertelrud

Assignees

@neonichu neonichu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@neonichu @tomerd @MaxDesiatov