Skip to content

[ScanDependency] Move binary module validation into scanner #72291

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
Merged
merged 2 commits into from
Apr 5, 2024
Merged

[ScanDependency] Move binary module validation into scanner #72291

merged 2 commits into from
Apr 5, 2024

Conversation

cachemeifyoucan
Copy link
Contributor

Improve swift dependency scanner by validating and selecting dependency module into scanner. This provides benefits that:

  • Build system does not need to schedule interface compilation task if the candidate module is picked, it can just use the candidate module directly.
  • There is no need for forwarding module in the explicit module build. Since the build system is coordinating the build, there is no need for the forwarding module in the module cache to avoid duplicated work,
  • This also correctly supports all the module loading modes in the dependency scanner. This is achieved by only adding validate and up-to-date binary module as the candidate module for swift interface module dependency. This allows caching build to construct the correct dependency in the CAS. If there is a candidate module for the interface module, dependency scanner will return a binary module dependency in the dependency graph.

rdar://123711823

@cachemeifyoucan
Copy link
Contributor Author

This is not ready to land yet because I need to find way to fix the tests and clean up some code. Want to collect feedback for the implementation before doing all the extra work.

benlangmuir
benlangmuir reviewed Mar 13, 2024
View reviewed changes
Copy link
Contributor

@benlangmuir benlangmuir left a comment

Choose a reason for hiding this comment

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

Approach looks reasonable to me, though I'm not an expert on this area of the code

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

Update: The legacy behavior is preserved with a hidden frontend flag
-legacy-scanner-behavior, while the output is mostly interchangeable
with new scanner behavior with prefer-interface module loading mode
except the candidate module will not be returned.

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

Rename the new flag name to -no-scanner-module-validation since it doesn't 100% preserve the old behavior but it is very very close, and delegate most of the module validation to swift-frontend and still generates forwarding module.

To clarify my previous comment, the behavior of the build should mostly stay the same (default to prefer-serialized loading mode) while the dependency scanning output from old compiler is very close to prefer-interface output except there is no candidate module reported.

@cachemeifyoucan cachemeifyoucan mentioned this pull request Mar 14, 2024
Merged
@cachemeifyoucan
Copy link
Contributor Author

Need swift-driver change: swiftlang/swift-driver#1563 so the tests there are not broken because the change in scanner causes swift-driver to schedule less jobs.

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

ping

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
[ScanDependency] Move binary module validation into scanner
0e12f20 
Improve swift dependency scanner by validating and selecting dependency
module into scanner. This provides benefits that:
* Build system does not need to schedule interface compilation task if
  the candidate module is picked, it can just use the candidate module
  directly.
* There is no need for forwarding module in the explicit module build.
  Since the build system is coordinating the build, there is no need for
  the forwarding module in the module cache to avoid duplicated work,
* This also correctly supports all the module loading modes in the
  dependency scanner.

This is achieved by only adding validate and up-to-date binary module as
the candidate module for swift interface module dependency. This allows
caching build to construct the correct dependency in the CAS. If there
is a candidate module for the interface module, dependency scanner will
return a binary module dependency in the dependency graph.

The legacy behavior is mostly preserved with a hidden frontend flag
`-no-scanner-module-validation`, while the scanner output is mostly
interchangeable with new scanner behavior with `prefer-interface` module
loading mode except the candidate module will not be returned.

rdar://123711823
@cachemeifyoucan
[DependencyScanning] Handle testable dependencies correctly
d4c90d6 
Teach scanner to pick and choose binary modules correctly based on if it
is testable import or not. Some situations that scanner need to be
careful when testable is involved:

* When it is a regular import, it should not import binary modules that
  are built with -enable-testing, it should prefer interfaces if that is
  available.
* When testable import, it should only load binary module and it should
  make sure the internal imports from binary modules are actually
  required for testable import to work.

If a testable import only find a regular binary module, dependency
scanner currently will just preceed with such module and leave the
diagnostics to swift-frontend, because the alternative (failed to find
module) can be confusing to users.

rdar://125914165
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

artemcm
artemcm reviewed Apr 5, 2024
View reviewed changes
lib/Serialization/SerializedModuleLoader.cpp
return std::make_error_code(std::errc::no_such_file_or_directory);
}

if (loadedModuleFile->isTestable() && !isTestableImport) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we currently (before this change) have clients refuse to load isTestable binary modules from a non-@testable imports or is that a semantic change?

In theory, a non-@testable import should be able to load such a module just fine, as it represents a super set of the same module built without build-for-testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can add in more dependency than needed which the user might or might not be able to satisfy.

Current explicit build will discover dependency from interface so it doesn't see those dependencies. I guess during forwarding module creation, it will see if it can satisfy all dependencies to see if it needs build from interface or create forwarding from testing module. That behavior is quite hard to support in this new model.

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test macOS platform

@cachemeifyoucan cachemeifyoucan merged commit 109c85e into swiftlang:main Apr 5, 2024
@cachemeifyoucan cachemeifyoucan mentioned this pull request Apr 16, 2024
Merged
This was referenced Jul 8, 2024
Merged
Merged
@artemcm artemcm mentioned this pull request Jul 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benlangmuir benlangmuir benlangmuir left review comments

@artemcm artemcm artemcm approved these changes

@akyrtzi akyrtzi Awaiting requested review from akyrtzi

@xymus xymus Awaiting requested review from xymus xymus is a code owner

@tshortli tshortli Awaiting requested review from tshortli tshortli is a code owner

@hborla hborla Awaiting requested review from hborla hborla is a code owner

@slavapestov slavapestov Awaiting requested review from slavapestov slavapestov is a code owner

@xedin xedin Awaiting requested review from xedin xedin is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cachemeifyoucan @benlangmuir @artemcm