Skip to content

[Dependency Scanning] Add ability to use the dependency scanner's import-prescan mode #717

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
Jun 16, 2021

Conversation

artemcm
Copy link
Contributor

@artemcm artemcm commented Jun 16, 2021

An ability to do a simple import pre-scan was added to the dependency scanner in swiftlang/swift#34028, with a corresponding libSwiftScan API added later. It parses input files looking for import statements and outputs an array of module names imported in the provided input source files.

This PR adds the ability to invoke this import pre-scan from the driver.

@artemcm artemcm requested review from davidungar and nkcsgexi June 16, 2021 19:38
@artemcm
Copy link
Contributor Author

artemcm commented Jun 16, 2021

@swift-ci please test

@artemcm artemcm force-pushed the PreScanCapability branch from 1d55311 to c98af5b Compare June 16, 2021 19:47
Copy link
Contributor

@davidungar davidungar left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM. There was one place I had to reread, and mentioned a nit there.

Comment on lines +116 to +120
// We generate full swiftc -frontend -scan-dependencies invocations in order to also be
// able to launch them as standalone jobs. Frontend's argument parser won't recognize
// -frontend when passed directly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice "why" comment!

}

private func sanitizeCommandForLibScanInvocation(_ command: inout [String]) {
// Remove the tool executable to only leave the arguments
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment leaves me wondering what the executable will be?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or why there doesn't need to be one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an explanation. Thanks for catching.

let moduleVersionedGraphMap: [ModuleDependencyId: [InterModuleDependencyGraph]]
if (!fallbackToFrontend) {

if !(try initSwiftScanLib()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Would be clearer to break up:

let isSwiftScanLibAvailable = !try initSwiftScanLib()
if isSwiftScanLibAvailable {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much clearer, indeed. Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

PS: Someday, I'd like to meet the llvm engineer who decided to use "true" for "hadError" and find out the rationale.

… mode

An ability to do a simple import pre-scan was added to the dependency scanner in swiftlang/swift#34028, with a corresponding libSwiftScan API added later.

This PR adds the ability to invoke this import pre-scan from the driver.
@artemcm artemcm force-pushed the PreScanCapability branch from c98af5b to e5d6e9f Compare June 16, 2021 21:42
@artemcm
Copy link
Contributor Author

artemcm commented Jun 16, 2021

@swift-ci please test

@artemcm
Copy link
Contributor Author

artemcm commented Jun 16, 2021

Thanks, @davidungar!

@davidungar
Copy link
Contributor

PPS: Glad the comments were helpful.

@artemcm artemcm merged commit b754520 into swiftlang:main Jun 16, 2021
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