Skip to content

Conversation

@cachemeifyoucan
Copy link
Contributor

@cachemeifyoucan cachemeifyoucan commented Jan 18, 2024

Previously, canImport lookup is not completely working with explicit module due to two issues:

  • For clang modules, canImport check still do a full modulemap lookup which is repeated work from scanner. For caching builds, this lookup cannot be performed because all modulemap and search path are dropped after scanning.
  • For swift module, if the canImport module was never actually imported later, this canImport check will fail during the actual compilation, causing different dependencies in the actual compilation.

To fix the problem, first unified the lookup method for clang and swift module, which will only lookup the module dependencies reported by scanner to determine if canImport succeed or not. Secondly, add all the successful canImport check modules into the dependency of the current module so this information can be used during actual compilation.

Note the behavior change here is that if a module is only checked in canImport but never imported still needs to be built. Comparing to implicit module build, this can bring in additional clang modules if they are only check inside canImport but should not increase work for swift modules (where binary module needs to be on disk anyway) or the most common usecase for canImport which is to check the same module before importing.

rdar://121082031

@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

@swift-ci please smoke test linux platform

@cachemeifyoucan
Copy link
Contributor Author

Note: this is not enough because you can write a much more complicated if clause for canImport. The only way that doesn't duplicate the work is to ask parser to remember all the checks. Updating the patch.

Previously, canImport lookup is not completely working with explicit
module due to two issues:
* For clang modules, canImport check still do a full modulemap lookup
  which is repeated work from scanner. For caching builds, this lookup
  cannot be performed because all modulemap and search path are dropped
  after scanning.
* For swift module, if the canImport module was never actually imported
  later, this canImport check will fail during the actual compilation,
  causing different dependencies in the actual compilation.

To fix the problem, first unified the lookup method for clang and swift
module, which will only lookup the module dependencies reported by
scanner to determine if `canImport` succeed or not. Secondly, add all
the successful `canImport` check modules into the dependency of the
current module so this information can be used during actual
compilation.

Note the behavior change here is that if a module is only checked in
`canImport` but never imported still needs to be built. Comparing to
implicit module build, this can bring in additional clang modules if
they are only check inside `canImport` but should not increase work for
swift modules (where binary module needs to be on disk anyway) or the
most common usecase for `canImport` which is to check the same module
before importing.

rdar://121082031
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

Note: this is not enough because you can write a much more complicated if clause for canImport. The only way that doesn't duplicate the work is to ask parser to remember all the checks. Updating the patch.

Fixed now and shouldn't not add any scanning overhead.

Copy link
Contributor

@artemcm artemcm left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks for the fix!

// RUN: echo "\"moduleName\": \"Swift\"," >> %/t/map.json
// RUN: echo "\"modulePath\": \"Swift.swiftmodule\"," >> %/t/map.json
// RUN: echo -n "\"moduleCacheKey\": " >> %/t/map.json
// RUN: cat %t/Swift.key >> %/t/map.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think this could be rewritten as a template file using split-file and then pipe it through sed to inject the strings you need. But I don't want to block the change on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it might be useful to have a python script to generate this file if this keeps happening. sed doesn't work well here either for how many places need to be replaced.

@cachemeifyoucan cachemeifyoucan merged commit 746de8b into swiftlang:main Jan 18, 2024
cachemeifyoucan added a commit to cachemeifyoucan/swift that referenced this pull request Jan 19, 2024
…ang#70974

When clang submodule is used in `canImport` check, it should return the
top level module as dependency, not the submodule. This fixes a
regression after swiftlang#70974 that causes dependency scanning to fail due to
failed submodule lookup during dependency scanning.
cachemeifyoucan added a commit that referenced this pull request Jan 20, 2024
[DependencyScanning] Fix clang submodule canImport check after #70974
cachemeifyoucan added a commit to cachemeifyoucan/swift that referenced this pull request Feb 29, 2024
This reverts following changes related to the new canImport check logic:

* Revert "[Caching] Fix versioned canImport check for swift module"

This reverts commit 1ababc0.

* Revert "[DependencyScanning] Fix clang submodule canImport check after swiftlang#70974"

This reverts commit 81aa7b1.

* Revert "[ExplicitModule] Fix `canImport` lookup for swift explicit module build"

This reverts commit 4fb7abc.
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