Skip to content

Allow for non-external lookup of libSwiftScan symbols and centralize the scanning instance use in the driver. #1696

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
merged 1 commit into from
Sep 20, 2024

Conversation

artemcm
Copy link
Contributor

@artemcm artemcm commented Sep 17, 2024

  • The driver now holds a reference to the swiftScanLibInstance which is shared with the interModuleDependencyOracle, but is also used for target info and supported compiler feature queries. This means a single SwiftScan instance is shared across all uses in a given driver instance.
  • Allow SwiftScan to be instantiated without a path to an external libSwiftScan.dylib, which will cause it to dlopen with a NULL argument, and expect the scanner symbols to be found in the image that the driver code is a part of. This is useful for when compiler-based tooling (SourceKit) links the driver directly and uses its C API.

@artemcm
Copy link
Contributor Author

artemcm commented Sep 17, 2024

@swift-ci test

@artemcm artemcm force-pushed the AllowInProcessDriverSwiftScan branch 2 times, most recently from c8e7904 to 409725d Compare September 17, 2024 21:53
@artemcm
Copy link
Contributor Author

artemcm commented Sep 17, 2024

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented Sep 18, 2024

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented Sep 18, 2024

@swift-ci test Windows platform

@artemcm artemcm marked this pull request as ready for review September 18, 2024 17:06
Copy link
Contributor

@cachemeifyoucan cachemeifyoucan left a comment

Choose a reason for hiding this comment

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

LGTM in general.

@@ -209,10 +210,16 @@ public class InterModuleDependencyOracle {
}
}

private var hasScannerInstance: Bool { self.swiftScanLibInstance != nil }
var hasScannerInstance: Bool { self.swiftScanLibInstance != nil }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be true when you have the integrated version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because even in the integrated version the oracle technically does have an instance of SwiftScan which is functional for using the API calls we expect it to have, in both cases. And this variable is only used for preconditions before relying on said APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

documentation comment saying that please :)

@artemcm artemcm force-pushed the AllowInProcessDriverSwiftScan branch from 2390926 to b47a2db Compare September 18, 2024 18:07
@artemcm
Copy link
Contributor Author

artemcm commented Sep 18, 2024

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented Sep 18, 2024

@swift-ci test Windows platform

1 similar comment
@artemcm
Copy link
Contributor Author

artemcm commented Sep 18, 2024

@swift-ci test Windows platform

@artemcm artemcm force-pushed the AllowInProcessDriverSwiftScan branch from b47a2db to 7d3cd74 Compare September 18, 2024 19:42
@artemcm
Copy link
Contributor Author

artemcm commented Sep 18, 2024

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented Sep 18, 2024

@swift-ci test Windows platform

1 similar comment
@artemcm
Copy link
Contributor Author

artemcm commented Sep 18, 2024

@swift-ci test Windows platform

@artemcm artemcm force-pushed the AllowInProcessDriverSwiftScan branch from 7d3cd74 to 943d96f Compare September 19, 2024 16:11
@artemcm
Copy link
Contributor Author

artemcm commented Sep 19, 2024

@swift-ci test Windows platform

@artemcm
Copy link
Contributor Author

artemcm commented Sep 19, 2024

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented Sep 19, 2024

@swift-ci test Windows platform

…the scanning instance use in the driver.

- The driver now holds a reference to the 'swiftScanLibInstance' which is shared with the 'interModuleDependencyOracle', but is also used for target info and supported compiler feature queries. This means a single SwiftScan instance is shared across all uses in a given driver instance.
- Allow SwiftScan to be instantiated *without* a path to an external 'libSwiftScan.dylib', which will cause it to 'dlopen' with a NULL argument, and expect the scanner symbols to be found in the image that the driver code is a part of. This is useful for when compiler-based tooling (SourceKit) links the driver directly and uses its C API.
@artemcm artemcm force-pushed the AllowInProcessDriverSwiftScan branch from 943d96f to c5a58e0 Compare September 20, 2024 16:03
@artemcm
Copy link
Contributor Author

artemcm commented Sep 20, 2024

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented Sep 20, 2024

@swift-ci test Windows platform

@artemcm artemcm merged commit d616136 into swiftlang:main Sep 20, 2024
3 checks passed
@artemcm artemcm deleted the AllowInProcessDriverSwiftScan branch September 20, 2024 20:05
@drodriguez
Copy link
Contributor

I am seeing in https://ci.swift.org/job/swift-PR-Linux-smoke-test/15242/ this warning: Unable to locate libSwiftScan. Fallback to swift-frontend dependency scanner invocation. in the SwiftPM test ResourcesTests.testResourcesOutsideOfTargetCanBeIncluded when started a smoke PR test in swiftlang/swift#76616 (changing the tests), but looking at other failing PR testing like in swiftlang/swift#76621 (again a smoke test) making tests like PackageCommandTests.testCommandPluginDiagnostics and PackageCommandTests.testCommandPluginBuildLogs (https://ci.swift.org/job/swift-PR-Linux-smoke-test/15241).

@@ -83,16 +83,17 @@ public class InterModuleDependencyOracle {
}

/// Given a specified toolchain path, locate and instantiate an instance of the SwiftScan library
public func verifyOrCreateScannerInstance(fileSystem: FileSystem,
Copy link
Contributor

Choose a reason for hiding this comment

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

In future, would be good to leave in a deprecated alias to allow time to adjust clients.

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.

5 participants