Skip to content

Stop using llbuild's Swift compiler tool #6585

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 1 commit into from
May 22, 2023
Merged

Stop using llbuild's Swift compiler tool #6585

merged 1 commit into from
May 22, 2023

Conversation

neonichu
Copy link
Contributor

@neonichu neonichu commented May 18, 2023 •
edited
Loading

This allows us to take control of these compiler invocations and is a first step towards unifying all the various ways of generating compiler arguments that are currently in SwiftTargetBuildDescription.

This PR aims to keep the behavior of the existing tool as far as possible, the only known differences are single quotes instead of double for arguments with spaces and the fact that the output file map will be created eagerly.

resolves #6575

@neonichu neonichu self-assigned this May 18, 2023
@neonichu neonichu force-pushed the stop-using-llbuild-swift-compiler-tool branch from df7b84e to 26c2ab7 Compare May 18, 2023 21:03
@neonichu
Copy link
Contributor Author

This seems to basically work as a drop-in replacement, so that we can now control these compiler invocations fully without having to change llbuild.

I'm seeing 5 test failures locally which are all related to the fact that we now eagerly write out the output file map. Not sure yet how to fix, ideally we would write the file map prior to invoking the compiler, but that seems like a bigger and separatable change.

@neonichu neonichu force-pushed the stop-using-llbuild-swift-compiler-tool branch from 26c2ab7 to 1f92ffb Compare May 18, 2023 21:59
@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author 

' against pattern regex("swiftc.* -module-name special_tool .* \".*/more spaces/special tool/some file.swift\"")Test Case 'BasicTests.testSwiftPackageWithSpaces' failed (2.831 seconds)

hmmm, maybe single vs. double quotes?

@MaxDesiatov
Copy link
Contributor

Would you provide more context on this change? What are the benefits of switching to ShellTool here?

@neonichu
Copy link
Contributor Author

This will allow us to evolve how we're invoking the compiler independently of llbuild, e.g. #6573 wouldn't apply to any compiler commands that go through the SwiftCompilerTool currently. See also #6575 for some context.

More importantly, I think this is a good step towards cleaning up SwiftTargetBuildDescription which currently has several completely separate ways of constructing Swift commands and this is one reason why it needs to do that. We should eventually unify them all.

@neonichu neonichu force-pushed the stop-using-llbuild-swift-compiler-tool branch from 1f92ffb to 10cdb55 Compare May 19, 2023 18:43
@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

@swift-ci please smoke test windows

@neonichu
Copy link
Contributor Author

Looks like this is fully passing now, so going to force push once more to fix the commit message.

@neonichu
Stop using llbuild's Swift compiler tool
fb654e2 
This allows us to take control of these compiler invocations and is a first step towards unifying all the various ways of generating compiler arguments that are currently in `SwiftTargetBuildDescription`.

This PR aims to keep the behavior of the existing tool as far as possible, the only known differences are single quotes instead of double for arguments with spaces and the fact that the output file map will be created eagerly.

resolves #6575
@neonichu neonichu force-pushed the stop-using-llbuild-swift-compiler-tool branch from 10cdb55 to fb654e2 Compare May 19, 2023 18:55
@neonichu neonichu changed the title WIP: Stop using llbuild's Swift compiler tool Stop using llbuild's Swift compiler tool May 19, 2023
@neonichu neonichu marked this pull request as ready for review May 19, 2023 18:55
@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

@swift-ci please smoke test windows

@neonichu
Copy link
Contributor Author

We should probably give ClangTool the same treatment at some point, but that has less hard-coded behavior so might be fine.

@neonichu neonichu merged commit 706d1f1 into main May 22, 2023
@neonichu neonichu deleted the stop-using-llbuild-swift-compiler-tool branch May 22, 2023 16:38
neonichu added a commit that referenced this pull request Aug 23, 2023
@neonichu
WIP: swift-get-version
8be50bf 
The changes in #6585 meant that we are no longer using LLBuild's built-in tracking of Swift compiler versions. We are lacking the infrastructure to really use that for the same purpose since we are now running the Swift compiler as a shell-tool, so this is adding a poor man's version of that.

We have a task that writes the output of `swift -version` for a particular Swift compiler path to the build directory and all Swift tasks that are using that compiler depend on that file as an input. This should give us the desired behavior of the tasks re-running if the Swift version changes.

This PR passes the existing test suite, but I need to add some tests for the particular scenario of re-running if the version changes.

rdar://114047018
@neonichu neonichu mentioned this pull request Aug 23, 2023
Merged
neonichu added a commit that referenced this pull request Aug 23, 2023
@neonichu
Implement swift-get-version in SwiftPM
c14ce02 
The changes in #6585 meant that we are no longer using LLBuild's built-in tracking of Swift compiler versions. We are lacking the infrastructure to really use that for the same purpose since we are now running the Swift compiler as a shell-tool, so this is adding a poor man's version of that.

We have a task that writes the output of `swift -version` for a particular Swift compiler path to the build directory and all Swift tasks that are using that compiler depend on that file as an input. This should give us the desired behavior of the tasks re-running if the Swift version changes.

rdar://114047018
neonichu added a commit that referenced this pull request Aug 23, 2023
@neonichu
Implement swift-get-version in SwiftPM
2b35cad 
The changes in #6585 meant that we are no longer using LLBuild's built-in tracking of Swift compiler versions. We are lacking the infrastructure to really use that for the same purpose since we are now running the Swift compiler as a shell-tool, so this is adding a poor man's version of that.

We have a task that writes the output of `swift -version` for a particular Swift compiler path to the build directory and all Swift tasks that are using that compiler depend on that file as an input. This should give us the desired behavior of the tasks re-running if the Swift version changes.

rdar://114047018
neonichu added a commit that referenced this pull request Aug 23, 2023
@neonichu
Implement swift-get-version in SwiftPM
361cc78 
The changes in #6585 meant that we are no longer using LLBuild's built-in tracking of Swift compiler versions. We are lacking the infrastructure to really use that for the same purpose since we are now running the Swift compiler as a shell-tool, so this is adding a poor man's version of that.

We have a task that writes the output of `swift -version` for a particular Swift compiler path to the build directory and all Swift tasks that are using that compiler depend on that file as an input. This should give us the desired behavior of the tasks re-running if the Swift version changes.

rdar://114047018
neonichu added a commit that referenced this pull request Aug 23, 2023
@neonichu
Implement swift-get-version in SwiftPM
d9b8920 
The changes in #6585 meant that we are no longer using LLBuild's built-in tracking of Swift compiler versions. We are lacking the infrastructure to really use that for the same purpose since we are now running the Swift compiler as a shell-tool, so this is adding a poor man's version of that.

We have a task that writes the output of `swift -version` for a particular Swift compiler path to the build directory and all Swift tasks that are using that compiler depend on that file as an input. This should give us the desired behavior of the tasks re-running if the Swift version changes.

rdar://114047018
neonichu added a commit that referenced this pull request Aug 25, 2023
@neonichu
Implement swift-get-version in SwiftPM
5f48133 
The changes in #6585 meant that we are no longer using LLBuild's built-in tracking of Swift compiler versions. We are lacking the infrastructure to really use that for the same purpose since we are now running the Swift compiler as a shell-tool, so this is adding a poor man's version of that.

We have a task that writes the output of `swift -version` for a particular Swift compiler path to the build directory and all Swift tasks that are using that compiler depend on that file as an input. This should give us the desired behavior of the tasks re-running if the Swift version changes.

rdar://114047018
neonichu added a commit that referenced this pull request Aug 25, 2023
@neonichu
Implement swift-get-version in SwiftPM (#6840)
55936dc 
The changes in #6585 meant that we are no longer using LLBuild's built-in tracking of Swift compiler versions. We are lacking the infrastructure to really use that for the same purpose since we are now running the Swift compiler as a shell-tool, so this is adding a poor man's version of that.

We have a task that writes the output of `swift -version` for a particular Swift compiler path to the build directory and all Swift tasks that are using that compiler depend on that file as an input. This should give us the desired behavior of the tasks re-running if the Swift version changes.

rdar://114047018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxDesiatov MaxDesiatov MaxDesiatov approved these changes

@abertelrud abertelrud Awaiting requested review from abertelrud

@tomerd tomerd Awaiting requested review from tomerd

Assignees

@neonichu neonichu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SPM does not control the swift driver invocation in all cases
2 participants
@neonichu @MaxDesiatov