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.

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

[Dependency Scanning] Query package-only dependencies from adjacent binary modules when necessary #76321

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

artemcm
Copy link
Contributor

@artemcm artemcm commented Sep 6, 2024

When '.package.swiftinterface' loading ('-experimental-package-interface-load') is disabled and when '-scanner-module-validation' is disabled, the scanner defaults to locating the non-package textual interface and may specify its adjacent binary module as a valid candidate binary module to use. If said candidate is up-to-date and ends up getting used, and belongs to the same package as the loading Swift source, then the source compilation may attempt to load its package-only dependencies. Since the scanner only parsed the non-package textual interface, those dependencies are not located and specified as inputs to compilation. This change causes the scanner, in such cases, to also lookup package-only dependencies in adjacent binary Swift modules of textual Swift module dependencies, if such dependency belongs to the same package as the source target being scanned.

Resolves rdar://135215789

…inary modules when necessary

When '.package.swiftinterface' loading ('-experimental-package-interface-load') is disabled and when '-scanner-module-validation' is disabled, the scanner defaults to locating the non-package textual interface and may specify its adjacent binary module as a valid candidate binary module to use. If said candidate is up-to-date and ends up getting used, and belongs to the same package as the loading Swift source, then the source compilation may attempt to load its package-only dependencies. Since the scanner only parsed the non-package textual interface, those dependencies are not located and specified as inputs to compilation. This change causes the scanner, in such cases, to also lookup package-only dependencies in adjacent binary Swift modules of textual Swift module dependencies, if such dependency belongs to the same package as the source target being scanned.

Resolves rdar://135215789
@artemcm
Copy link
Contributor Author

artemcm commented Sep 6, 2024

@swift-ci test

Copy link
Contributor

@nkcsgexi nkcsgexi left a comment

Choose a reason for hiding this comment

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

This makes sense to me. Thank you for the quick fix!

@cachemeifyoucan
Copy link
Contributor

cachemeifyoucan commented Sep 6, 2024

I don't like this direction we are going. The reason this is not good is because:

  • This assumes the package dependency can be read from the module even when the version mismatched, which we don't guarantee that.
  • The dependency is different based on if the binary module exists or not. This means the incremental build has problem invalidating the previous dependency.
  • If the candidate module is not taken, there are extra dependencies brought in, but project might expect more interface than provided from the module built from interface.

I am hoping the decision is that if -package-name is used and package interface is disabled, the only valid dependency is binary module. I don't think I got the support for that the last time I brought this up.

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.

The change generally looks good to me, except the problem that we don't know if the package name and dependencies can be read successfully from an invalid/out-of-date module.

@artemcm
Copy link
Contributor Author

artemcm commented Sep 6, 2024

I don't like this direction we are going. The reason this is not good is because:

  • This assumes the package dependency can be read from the module even when the version mismatched, which we don't guarantee that.
  • The dependency is different based on if the binary module exists or not. This means the incremental build has problem invalidating the previous dependency.
  • If the candidate module is not taken, there are extra dependencies brought in, but project might expect more interface than provided from the module built from interface.

I am hoping the decision is that if -package-name is used and package interface is disabled, the only valid dependency is binary module. I don't think I got the support for that the last time I brought this up.

Broadly, this direction is going away two-fold in the very near future: -scanner-module-validation becoming default and -experimental-package-interface-load also becoming default. The combination of these would mean this kind of workaround is not necessary.
Otherwise I am also onboard with making the binary module the only valid dependency here, but it seems that we're moving in a direction where that's moot.

@nkcsgexi
Copy link
Contributor

nkcsgexi commented Sep 6, 2024

Per Steven's point, can we pull the package import from the package interface regardless? We should be successful even the binary module is out of date.

@nkcsgexi
Copy link
Contributor

nkcsgexi commented Sep 6, 2024

Thinking more about this, out of date binary module isn't a big problem here because we have to use the private Swift textual interface on that case anyway. Thus missing some package dependencies isn't the end of the world.

@artemcm
Copy link
Contributor Author

artemcm commented Sep 6, 2024

@swift-ci test macOS platform

@cachemeifyoucan
Copy link
Contributor

Sure, I can onboard with this for a temporary fix. My worry is simply projects will start to rely on this behavior.

Prefer just tell people to turn on package interfaces

@artemcm artemcm merged commit b70824c into swiftlang:main Sep 9, 2024
5 checks passed
@artemcm artemcm deleted the PackageOnlyAdjacentScanFix branch September 9, 2024 16:53
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