Add support for outputting dependency manifest files, used by ninja and make #178

rgeary1 · 2015-01-23T17:54:32Z

Use --manifest-file=somefile.d to output the dependency manifest.
This file will contain a list of files which were read by protoc as part
of creating the output files. It doesn't include the plugin inputs if
plugins are used, that could be a later extension.
The manifest file is in the format : . The
manifest file format only allows you to specify one output file, which
isn't a problem as it's used to detect input changes in order to detect
when to rerun the protoc command. The output file used in the manifest
is the manifest filename itself; to use this in ninja you should declare
the manifest file as the first output as well as the depfile input.

…nd make Use --manifest-file=somefile.d to output the dependency manifest. This file will contain a list of files which were read by protoc as part of creating the output files. It doesn't include the plugin inputs if plugins are used, that could be a later extension. The manifest file is in the format <output file>: <input files>. The manifest file format only allows you to specify one output file, which isn't a problem as it's used to detect input changes in order to detect when to rerun the protoc command. The output file used in the manifest is the manifest filename itself; to use this in ninja you should declare the manifest file as the first output as well as the depfile input.

xfxyjwf · 2015-01-23T19:07:39Z

@TeBoring to take a look. We got internal feature requests for this as well. Need to find a solution that works both for internal users and opensource users.

TeBoring · 2015-01-31T23:02:57Z

I don't quite understand what exactly do you want.
Could you give several examples showing what the output file look like given input?

TeBoring · 2015-02-01T00:10:30Z

src/google/protobuf/compiler/command_line_interface.cc

+    output_filename = output_filename.substr(2);
+  }
+  ss << output_filename << ": ";
+  for (set<const FileDescriptor*>::const_iterator it = already_seen.begin(); it != already_seen.end(); ++it ) {


Should it iterate on file_set.file()?

Yes this should be file_set. Originally I was calling GetSourceLocation, which is a private member of FileDescriptor.

rgeary1 · 2015-02-02T23:18:17Z

The makefile depency file output contents is

<path to output file>: <path to input file1> <path to input file2> ...

you can separate lines for readability with a \ character followed by a newline character

For examples, you can generate this file for a number of situations with gcc with the flags "-MMD -MF test.d".
The only restriction I know of (for ninja, and maybe make) is there can only be one output file. But since the purpose of the depfile is to invalidate the state of all the outputs if one input changes, then it seems appropriate to give the path to the depfile itself as the output file, and let the build system enumerate the output files.

googlebot · 2015-02-02T23:18:18Z

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

TeBoring · 2015-02-03T18:07:55Z

Could you sign the cla first? Thanks

TeBoring · 2015-02-03T18:55:36Z

Internally, our need is a little bit different. We don't give protoc the name for manifest file. protoc will generated a .d file for each input file, which records dependencies of just that single input file.
I have also added your feature into that implementation. So, if you use -m, protoc will generate .d file for each proto file. If you use -mFILE, protoc will write all dependencies into FILE.
I have also mentioned this pull request in my code. You don't need to change your code, but I still need your CLA. Does it sound good to you?

rgeary1 · 2015-02-03T21:28:49Z

Sounds good. Out of interest, which directory do you output the files to
if you just specify -m? I can't see your pull request - does the depfile
generated by your code look identical to mine?
I'm working on getting the CLA, I developed this as part of my employment
so I presume our company needs to sign it. Should happen, just will take
some time. My guess is a week.

On 3 February 2015 at 13:55, Paul Yang notifications@github.com wrote:

Internally, our need is a little bit different. We don't give protoc the
name for manifest file. protoc will generated a .d file for each input
file, which records dependencies of just that single input file.
I have also added your feature into that implementation. So, if you use
-m, protoc will generate .d file for each proto file. If you use -mFILE,
protoc will write all dependencies into FILE.
I have also mentioned this pull request in my code. You don't need to
change your code, but I still need your CLA. Does it sound good to you?

—
Reply to this email directly or view it on GitHub
#178 (comment).

TeBoring · 2015-02-03T22:20:03Z

If you just have -m, it will generate file in the same directory as the input file.
The content's format should be the same. I implemented internally, but it will be visible in our next opensource release.
But I have a question, do you expect all paths to be absolute paths in the following example?

<path to output file>: <path to input file1> <path to input file2> ...

rgeary1 · 2015-02-04T02:07:26Z

We have many users who have read-only access to some source directories. We're not intending to use the -m multi-file option, but you may want to consider making the multi-output directory explicit.

Depfile syntax : The output path should match the format expected by the build system, which will only be visible via the command line. If it's specified as relative on the cmdline, the output path needs to be relative. If it's specified as absolute on the cmdline it needs to be absolute in the depfile. It's the same for the input files.

TeBoring · 2015-02-04T02:36:47Z

Some files are not input files, but they are included by other input files.
How about their path?

On Tuesday, February 3, 2015, Richard Geary notifications@github.com
wrote:

We have many users who have read-only access to some source directories.
We're not intending to use the -m multi-file option, but you may want to
consider making the multi-output directory explicit.

Depfile syntax : The output path should match the format expected by the
build system, which will only be visible via the command line. If it's
specified as relative on the cmdline, the output path needs to be relative.
If it's specified as absolute on the cmdline it needs to be absolute in the
depfile. It's the same for the input files.

—
Reply to this email directly or view it on GitHub
#178 (comment).

rgeary1 · 2015-02-04T02:46:30Z

The most general solution would be to provide an option to the user. The option would specify a root path for depfile paths, and all paths under the root would be output as relative paths. If the option was not specified, the paths would be absolute.

I realize I've just proposed adding several new command line options to protoc; some projects resist command line option bloat, some seem to embrace it (eg. "man gcc"!). If I had to choose, I'd say use absolute paths always since it's unambiguous.

TeBoring · 2015-02-04T03:24:42Z

I prefer to use absolute path always.

On Tuesday, February 3, 2015, Richard Geary notifications@github.com
wrote:

The most general solution would be to provide an option to the user. The
option would specify a root path for depfile paths, and all paths under the
root would be output as relative paths. If the option was not specified,
the paths would be absolute.

I realize I've just proposed adding several new command line options to
protoc; some projects resist command line option bloat, some seem to
embrace it (eg. "man gcc"!). If I had to choose, I'd say use absolute paths
always since it's unambiguous.

—
Reply to this email directly or view it on GitHub
#178 (comment).

…he flag "--dependency_manifest_out=FILE", protoc will write dependencies of input proto files into FILE. In FILE, the format will be <full path to FILE>: <full path to 1st proto>\\\n <full path to 2nd proto> ... This cl is based on protocolbuffers#178

TeBoring · 2015-02-04T06:09:10Z

We just found the feature you implemented is enough for internal use. So instead, I want to clean up this pull request. I sent another pull request: #193 to clean the code.
It pulled your change and made change on it directly. After you signed cla, I will merge #193 instead of #178 .
Please take a look at #193. Thanks.

rgeary1 · 2015-03-04T12:33:25Z

Signed the CLA, Tower Research Capital

TeBoring · 2015-03-05T04:12:50Z

@rgeary1 Could you add your email (the one associated with this pull request) to tower-research-google-open-source-contributors@googlegroups.com

googlebot · 2015-03-05T13:39:47Z

CLAs look good, thanks!

xfxyjwf assigned xfxyjwf and TeBoring and unassigned xfxyjwf Jan 23, 2015

TeBoring reviewed Feb 1, 2015
View reviewed changes

googlebot added the cla: no label Feb 2, 2015

TeBoring mentioned this pull request Feb 4, 2015

Implement a feature to generate a dependency file #193

Merged

googlebot added cla: yes and removed cla: no labels Mar 5, 2015

TeBoring merged commit 532c941 into protocolbuffers:master Mar 8, 2015

google-admin unassigned TeBoring Aug 21, 2018

xfxyjwf assigned TeBoring Aug 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for outputting dependency manifest files, used by ninja and make #178

Add support for outputting dependency manifest files, used by ninja and make #178

rgeary1 commented Jan 23, 2015

xfxyjwf commented Jan 23, 2015

TeBoring commented Jan 31, 2015

TeBoring Feb 1, 2015

rgeary1 Feb 2, 2015

rgeary1 commented Feb 2, 2015

googlebot commented Feb 2, 2015

TeBoring commented Feb 3, 2015

TeBoring commented Feb 3, 2015

rgeary1 commented Feb 3, 2015

TeBoring commented Feb 3, 2015

rgeary1 commented Feb 4, 2015

TeBoring commented Feb 4, 2015

rgeary1 commented Feb 4, 2015

TeBoring commented Feb 4, 2015

TeBoring commented Feb 4, 2015

rgeary1 commented Mar 4, 2015

TeBoring commented Mar 5, 2015

googlebot commented Mar 5, 2015

Add support for outputting dependency manifest files, used by ninja and make #178

Add support for outputting dependency manifest files, used by ninja and make #178

Conversation

rgeary1 commented Jan 23, 2015

xfxyjwf commented Jan 23, 2015

TeBoring commented Jan 31, 2015

TeBoring Feb 1, 2015

Choose a reason for hiding this comment

rgeary1 Feb 2, 2015

Choose a reason for hiding this comment

rgeary1 commented Feb 2, 2015

googlebot commented Feb 2, 2015

TeBoring commented Feb 3, 2015

TeBoring commented Feb 3, 2015

rgeary1 commented Feb 3, 2015

TeBoring commented Feb 3, 2015

rgeary1 commented Feb 4, 2015

TeBoring commented Feb 4, 2015

rgeary1 commented Feb 4, 2015

TeBoring commented Feb 4, 2015

TeBoring commented Feb 4, 2015

rgeary1 commented Mar 4, 2015

TeBoring commented Mar 5, 2015

googlebot commented Mar 5, 2015