Allow optional filename argument for --dep-info #11046
Conversation
| let files: ~[@str] = sess.codemap.files.iter() | ||
| .filter_map(|fmap| if fmap.is_real_file() { Some(fmap.name) } else { None }) | ||
| .collect(); | ||
| let deps_filename = outputs[0].with_extension("d"); |
There was a problem hiding this comment.
This seems odd to me, why would we specifically bless the first output filename?
There was a problem hiding this comment.
Specifically, I don't think there's any reason we should only have a dependency file listed for only the first output and then inside the file it has the correct dependencies for all the outputs.
There was a problem hiding this comment.
From what I understand, multiple output files are always produced from the same result of phase 1 to 5 (parse, translate, optimize). So multiple outputs contain the same object code, just differently linked in phase 6. I.e. they are based on the same sources, same dependencies, even the same object code from the same rustc run, so they can as well use the same deps file. Make requires to specify the deps for every target file, thus the multiple lines. However I think the deps file could be squashed to a single line and specify dependencies for all targets in a single rule (target1 target2: dep1 dep2 dep3).
Multiple output files currently only happen if you use more than one of --dylib, --rlib or --staticlib. Even then, all output files are based on the same base filename. So producing a dep-info file for every output by replacing the extension of the output filename with .d would end up in the same dep-info file anyway.
I agree that using outputs[0] doesn't look right. It looks like the first output would be handled special. I should probably change it to base the filename on outputs.out_filename (outputs from function parameters) which makes it more clear what happens.
There was a problem hiding this comment.
updated and rebased
|
Updated and rebased. Default deps filename is based on crate source file like before. With an optional argument to --dep-info, an arbitrary filename can be used. |
| LIBEXT=so | ||
| endif | ||
|
|
||
| $(TMPDIR)/libfoo-b517899a-0.1.$(LIBEXT): |
There was a problem hiding this comment.
Now that we have --crate-file-name, this should probably be using the output of that.
LIB = $(shell $(RUSTC) --crate-file-name lib.rs)
$(LIB):
...
|
This looks great to me! A few minor comments and this should be good to go. |
|
Ok, changed the code to early return when there's no deps file to write. Looks better now indeed. I also added a warning if --dep-info is used without a filename when compiling stdin. Also added a few comments and changed the tests to use --crate-file-name. |
|
That's weird. I just built and ran tests on a 64bit Ubuntu VM with exactly the same configure options bors uses and it worked fine. |
|
I found it a little difficult to wade through the debug output of make, but if you push a version without that debugging (or with some extra debugging), I can push code to our try servers which can help you try to debug what's going on. |
|
I enabled debug logging for the first make run as well and disabled the internal built-in rules (they aren't needed for this Makefile and turning them off results in greatly reduced log output). The 1st make runs with updated dependency (touched foo.rs) to ensure that the lib is rebuild. The 2nd make ensures that the lib is not build again if dependencies weren't updated. The 2nd make failed because it did rebuild the lib even if it shouldn't. Strange thing is, that it rebuilt the lib because it didn't exist, which shouldn't happen at all. Since it worked before, my guess would be that either the 2nd make is expecting a different filename than the 1st produced (i.e. --lib produced a different file than --crate-file-name prints out), or maybe there's an issue with relative paths and/or the cwd (since the previous version used absolute paths) |
|
The problem occurred because Makefile.foo used just the filename without a directory in its first rule. I'm almost sure that --crate-file-name included the directory before, but it seems to only contain the filename without a path now. This change may be related to #11077. Is --crate-file-name intended to include the directory or should it print just the name of the file? I changed this PR to work with the current behavior of --crate-file-name and verified that run-make tests succeed on Linux 64bit and OSX 64bit. |
Using --dep-info writes Makefile-compatible dependency info to a file that is by default named based on the crate source filename. This adds an optional string argument to the --dep-info option which allows to write dependency info to an arbitrary filename. cc #10698
New lint [`iter_skip_zero`] Idea came from my contribution to `unnecessary_cast` recently. Sorry about that 😅 Could be an issue if somebody manually implements `skip`, but I don't think anybody will (and this would be an issue with other lints too, afaik) changelog: New lint [`iter_skip_zero`]
Using --dep-info writes Makefile-compatible dependency info to a file that is by default named based on the crate source filename. This adds an optional string argument to the --dep-info option which allows to write dependency info to an arbitrary filename.
cc #10698