Skip to content

[embedded] A number of changes to support embedded Swift. #1455

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

Merged

Conversation

zoecarver
Copy link
Contributor

No description provided.

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test

@zoecarver zoecarver changed the title [embeded] A number of changes to support embeded Swift. [embedded] A number of changes to support embedded Swift. Sep 26, 2023
Comment on lines 369 to 372
try commandLine.appendLast(.disableAutolinkingRuntimeCompatibility, from: &parsedOptions)
try commandLine.appendLast(.runtimeCompatibilityVersion, from: &parsedOptions)
try commandLine.appendLast(.disableAutolinkingRuntimeCompatibilityDynamicReplacements, from: &parsedOptions)
try commandLine.appendLast(.disableAutolinkingRuntimeCompatibilityConcurrency, from: &parsedOptions)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should drop this change as we probably still want to let users configure these options manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

All we need is to make sure these flags don't passed by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sort of feel like we should pass -runtime-compatibility-version none by default. But this seems to work 🤷

let diags = DiagnosticsEngine()
var driver = try Driver(args: ["swiftc", "-target", "arm64-apple-macosx10.13", "test.swift", "-enable-experimental-feature", "Embedded", "-parse-as-library", "-o", "a.out", "-module-name", "main"], diagnosticsEngine: diags)
_ = try driver.planBuild()
XCTAssertTrue(diags.diagnostics.first!.message.text == "Whole module optimization (wmo) must be enabled with embedded Swift.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this assert against the actual variable instead of hardcoded text? That way tests won't break if diagnostic text needs to change.

Suggested change
XCTAssertTrue(diags.diagnostics.first!.message.text == "Whole module optimization (wmo) must be enabled with embedded Swift.")
XCTAssertTrue(diags.diagnostics.first!.message.text == error_need_wmo_embedded.message.text)

@zoecarver
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

thanks!

@zoecarver
Copy link
Contributor Author

@MaxDesiatov I can't follow you test suggestion (even though I think it's better) because of access levels:

 error: 'error_no_objc_interop_embedded' is inaccessible due to 'internal' protection level

Seems strings are how other diags are tested.

@zoecarver
Copy link
Contributor Author

@swift-ci please test

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Oct 2, 2023

I can't follow you test suggestion (even though I think it's better) because of access levels:

@zoecarver Import that module with diagnostics as @testable, that will bring internal declarations into scope.

@zoecarver
Copy link
Contributor Author

Aha! Thank you, Max!

@zoecarver
Copy link
Contributor Author

@swift-ci please test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@zoecarver zoecarver merged commit 86b57cc into swiftlang:main Oct 2, 2023
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