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

Response files #217

Merged
merged 4 commits into from
Feb 23, 2012
Merged

Response files #217

merged 4 commits into from
Feb 23, 2012

Conversation

PetrWolf
Copy link
Contributor

This pull request adds support for response files to ninja.

Response files are common practice on Windows, where the maximal length of a command line is limited. In a nutshell, this change allows ninja to create response files using two new syntax elements (rspfile and rspfile_content) in rules definition.

For details see:

@evmar
Copy link
Collaborator

evmar commented Feb 10, 2012

This looks great. The only piece I don't understand is the "incl_rsp_file" parameter to EvaluateCommand(). What is it needed for? It appears to be part of the build log key, is it just for that?

@syntheticpp
Copy link
Contributor

What about renaming 'rspfile_content'? It looks a bit ugly besides the other rule variables.
Maybe 'rspin'?

@PetrWolf
Copy link
Contributor Author

Thanks for comments!

RE incl_rsp_file: Yes, it's only used to get the whole command line including the contents of the rsp file, so that it can be stored (and compared) in the log. Maybe a specific method (EvaluateFullCommand) or a different name of the attribute would make more sense. What do you think?

RE typos, whitespaces: Will do.

RE rspfile_content/rspin: It's more of a "content" (the file can contain anything) then "in" (a list of direct dependencies) really. So rspfile_content makes sense to me, but I don't mind changing it to something nicer. Any suggestions?

@syntheticpp
Copy link
Contributor

Maybe "rspcontent"?

Another point: Is it by design that no temporary filenames are used, or is it a missing feature?

@PetrWolf
Copy link
Contributor Author

The names of the rsp files are fixed (not temporary) by design - it is easy for the generator of a .ninja file to make up such name (e.g. name of the output + ".rsp"). This makes investigations of failures as well as internal handling of build log entries easier.

@PetrWolf
Copy link
Contributor Author

Added some updates based on comments above.

Please let me now if further changes are required.

@syntheticpp
Copy link
Contributor

OK, a known name is better than a generated one by the system.

@syntheticpp
Copy link
Contributor

But what I don't like is that always a response file is used, even when the content is very small.
Couldn't be checked if the content exceeds a limit, and only then a file is created?
The generator couldn't know the exact length of the content.

@PetrWolf
Copy link
Contributor Author

I believe this choice does belong to the generator - to be done once (even if it may not be 100% precise) and recorded in the .ninja files.

Note that it's not only a question of (not) creating the response file. We're talking about two essentially different commands here:

  • one without the @rspfile option and with all other options in it
  • the other one with some @rspfile options and with some options in a file.

@syntheticpp
Copy link
Contributor

Yes, that' the problem, you need two rules for the same command just to support response files.

@philipcraig
Copy link
Contributor

This "problem" is a reflection of the true state of the world. They correctly are two different commands and rules.

@syntheticpp
Copy link
Contributor

I had a look at the CMake code, and indeed, they check for response files.

So, I would rename rspfile_content into rspcontent or something similar and merge the patch.

@evmar
Copy link
Collaborator

evmar commented Feb 23, 2012

I'm gonna merge this as-is and we can discuss it further from there.

evmar added a commit that referenced this pull request Feb 23, 2012
@evmar evmar merged commit 18af810 into ninja-build:master Feb 23, 2012
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