Skip to content

Use response files when running subcommands if needed #57

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

Merged
merged 5 commits into from
Jan 25, 2020

Conversation

owenv
Copy link
Contributor

@owenv owenv commented Jan 7, 2020

Now, when a job is created, it can optionally indicate that it supports using a response file to pass its arguments. Then, ArgsResolver is responsible for checking the argument length and writing out a response file if needed. This ensures that if ArgsResolver lengthens the arguments, this is accounted for properly.

I'm fairly happy with this design, but there's one issue in how it interacts with LLBuild, which is why this is a WIP for now. A couple of the integration tests which pass 500k args run very slowly (~3-4 minutes each). It looks like most of the time is spent in JSONDecoder objc bridging (at least on macOS), which is used to serialize Jobs. This might need to be switched to a faster encoder/decoder.

This PR also makes a few changes to response file tokenization to more closely match the integrated driver.

With this change, two response file integration tests still fail: one due to different sets of files being passed with -primary-file, and another because the c++ and swift drivers use a different tmp dir layout.

@owenv owenv marked this pull request as ready for review January 14, 2020 17:50
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 looking good, thank you!

@DougGregor
Copy link
Member

@swift-ci please test

@Catfish-Man
Copy link

Ideally we should speed up JSONDecoder rather than switching away from it, at least initially.

@owenv
Copy link
Contributor Author

owenv commented Jan 19, 2020

That makes sense to me, especially since it's going to be awhile before the slow tests are up and running on CI builds. Even if llbuild is just working with byte strings, it's probably worth keeping things human-readable for debugging purposes.

@DougGregor DougGregor merged commit 90bc104 into swiftlang:master Jan 25, 2020
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.

4 participants