Skip to content

Propagate Explicit Module Build command arguments to compile jobs. #113

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 10 commits into from
Jun 8, 2020
Merged

Propagate Explicit Module Build command arguments to compile jobs. #113

merged 10 commits into from
Jun 8, 2020

Conversation

@artemcm
Copy link
Contributor

@artemcm artemcm commented Jun 1, 2020 •
edited
Loading

This PR adds the compile command argument interface for Explicit Module Builds, as specified in: swiftlang/swift#32125

  • -disable-implicit-swift-modules and -Xcc -Xclang -Xcc -fno-implicit-modules are passed to all compile jobs to prevent the frontend from building any modules implicitly and diagnose if they have to.
  • When building a module, the compile commands are passed paths to the module's direct dependencies with -Xcc -Xclang -Xcc -fmodule-map-file= and -Xcc -Xclang -Xcc -fmodule-file= and -swift-module-file=.
  • Add -experimental-explicit-module-build option to request that the driver must attempt to perform a full explicit-module-build compile.

For now, I include a test that verifies that the commands are generated as expected w.r.t. their dependencies. Once the frontend puzzle piece is landed, we should be close to getting a full end-to-end test.

This PR subsumes #112.

@artemcm artemcm requested a review from nkcsgexi June 1, 2020 19:38
@artemcm artemcm mentioned this pull request Jun 1, 2020
Closed
@artemcm artemcm force-pushed the ExplicitModuleBuild branch from da04575 to 8dadcc7 Compare June 1, 2020 19:43
artemcm
artemcm commented Jun 1, 2020
View reviewed changes
artemcm
artemcm commented Jun 1, 2020
View reviewed changes
Sources/SwiftDriver/Jobs/Planning.swift

// If asked, add jobs to precompile module dependencies
if parsedOptions.contains(.driverExplicitModuleBuild) ||
parsedOptions.contains(.driverPrintModuleDependenciesJobs) {
Copy link
Contributor Author

@artemcm artemcm Jun 1, 2020

Choose a reason for hiding this comment

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

driverPrintModuleDependenciesJobs makes this messy, but I plan on removing it in a followup and making tweaks to -print-driver-jobs to make debugging this a bit easier.

nkcsgexi
nkcsgexi reviewed Jun 1, 2020
View reviewed changes
nkcsgexi
nkcsgexi reviewed Jun 1, 2020
View reviewed changes
@DougGregor
Copy link
Member

DougGregor commented Jun 1, 2020

@swift-ci test

DougGregor
DougGregor approved these changes Jun 1, 2020
View reviewed changes
Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This is shaping up really nicely!

@shahmishal
Copy link
Member

shahmishal commented Jun 2, 2020

@swift-ci test

@DougGregor
Copy link
Member

DougGregor commented Jun 2, 2020

@swift-ci please test

@artemcm artemcm marked this pull request as ready for review June 2, 2020 16:48
@artemcm
Copy link
Contributor Author

artemcm commented Jun 2, 2020

@swift-ci test

2 similar comments
@artemcm
Copy link
Contributor Author

artemcm commented Jun 3, 2020

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented Jun 4, 2020

@swift-ci test

@DougGregor
Copy link
Member

DougGregor commented Jun 4, 2020

@swift-ci please test

1 similar comment
@artemcm
Copy link
Contributor Author

artemcm commented Jun 4, 2020

@swift-ci please test

@artemcm
Copy link
Contributor Author

artemcm commented Jun 4, 2020

The test added in this PR is failing in CI because it needs to invoke the compiler (dependency scanner) and I suspect that this is due to the way the driver currently does path resolution when looking for the compiler executable.
It attempts the following, in order, whichever method succeeds first:

  1. Try to find it with executable-specific environment variable
    (This is a typical development workflow, hence why "it works on my machine".)
  2. Try to find with xcrun -sdk macosx --find <executable>
  3. Try to search default search paths, which are just set to PATH
  4. Fallback to /usr/bin/.

Because CI points the driver to the latest toolchain with PATH, the driver ends up invoking the wrong compiler (system compiler found with xcrun, I suspect).

@DougGregor , we can make this work by doing either:

  1. Have CI export SWIFT_DRIVER_SWIFT_EXEC to point to the downloaded toolchain's bin.
  2. Swap the order 2 <-> 3 above.
    Am I right in thinking that the path resolution order is what it is for a good reason and we should pursue option (1)?

@DougGregor
Copy link
Member

DougGregor commented Jun 4, 2020

@DougGregor , we can make this work by doing either:

  1. Have CI export SWIFT_DRIVER_SWIFT_EXEC to point to the downloaded toolchain's bin.
  2. Swap the order 2 <-> 3 above.
    Am I right in thinking that the path resolution order is what it is for a good reason and we should pursue option (1)?

I think we should do (1), because we might want to fiddle with path resolution order for other reasons, but SWIFT_DRIVER_SWIFT_EXEC will remain the first point of override regardless of what we do.

Thank you!

@artemcm
Copy link
Contributor Author

artemcm commented Jun 5, 2020

@swift-ci please test

@artemcm artemcm force-pushed the ExplicitModuleBuild branch from 37c4725 to 4364bbb Compare June 5, 2020 16:54
@artemcm
Copy link
Contributor Author

artemcm commented Jun 5, 2020

@swift-ci please test

2 similar comments
@artemcm
Copy link
Contributor Author

artemcm commented Jun 8, 2020

@swift-ci please test

@artemcm
Copy link
Contributor Author

artemcm commented Jun 8, 2020

@swift-ci please test

@artemcm artemcm changed the title [WIP] Propagate Explicit Module Build command arguments to compile jobs. Propagate Explicit Module Build command arguments to compile jobs. Jun 8, 2020
@artemcm artemcm mentioned this pull request Jun 8, 2020
Merged
@DougGregor
Copy link
Member

DougGregor commented Jun 8, 2020

@swift-ci please test

@artemcm
Add an end-to-end test for the option to print compile jobs for modul…
65738b5 
…e dependencies

discovered by the fast dependency scanner.
artemcm added 9 commits June 8, 2020 11:57
@artemcm
Add a more-informative Driver error on Fast Dependency Scanner failure.
c8ff1e4
@artemcm
Specify module dependencies explicitly when pre-building module depen…
1ba27a7 
…dencies.
@artemcm
Propagate explicit module build commands and arguments to compile jobs
733b083
@artemcm
Adjust the clang module-file and disable-explicit-pcm interface to ma…
360db0e 
…tch the frontend
@artemcm
Add transitive module dependencies as explicit module arguments to co…
4cdd2d1 
…mpile jobs
@artemcm
Move addition of explicit module build arguments and inputs to addCom…
de75d02 
…monFrontendOptions
@artemcm
Use separate flag for -swift-module-file instead of EQ
497e029
@artemcm
Fix small explicit module build test file issues
50ff7fb 
- Explicitly import `Swift` in swiftinterface files
- Use quote header file imports instead of <>
@artemcm
Incorporate code-review formatting feedback.
5092bdb
@artemcm artemcm force-pushed the ExplicitModuleBuild branch from 56cc88e to 5092bdb Compare June 8, 2020 19:00
@artemcm
Copy link
Contributor Author

artemcm commented Jun 8, 2020

@swift-ci please test

@artemcm artemcm merged commit 2424c5a into swiftlang:master Jun 8, 2020
@artemcm artemcm deleted the ExplicitModuleBuild branch January 20, 2021 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@harlanhaskins harlanhaskins harlanhaskins left review comments

@DougGregor DougGregor DougGregor approved these changes

@cltnschlosser cltnschlosser cltnschlosser left review comments

@nkcsgexi nkcsgexi nkcsgexi left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@artemcm @DougGregor @shahmishal @harlanhaskins @cltnschlosser @nkcsgexi