1208566 - Generate dependency information for rust code

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ralph Giles (:rillian)]

Currently we can put the top-level module source (e.g. 'src/lib.rs') in SOURCES for rust code, and rustc will traverse included submodules and compile them as well. Mach doesn't know about this, however, so the crate won't be rebuild when files outside the SOURCES list change.

We should use `rustc --emit dep-info` to retrieve this information at the preprocessor step and make use of it in the mach build.

Comment 1

[Nathan Froyd [:froydnj]]

I will look at this.

Can rustc emit the dependencies in the same invocation as the actual compilation, or does emitting the dependencies need to be a completely different step?

Comment 2

[Alex Crichton [:acrichto]]

It can indeed do it all in the same step! You can do this via:

   rustc foo.rs --emit dep-info,link

And that'll generate libfoo.rlib and foo.d. You can also just do `--emit dep-info` if you want it as a separate step, however

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: attempt to have rustc emit proper dependency info for rust libraries

This version doesn't really work, mostly because rustc's idea of
emitting dep-info isn't rich enough for slotting cleanly into the Gecko
build system.  Doing something like:

  rustc --emit dep-info,link -o foo.a ...

causes rustc to complain that it's going to ignore -o, since we're
asking it to output multiple things.  It goes ahead and outputs foo.d in
the same directory as foo.a anyway, which isn't consistent with our
naming convention (foo.a.pp) or where our depfiles go (.deps, at least
on Linux).

OK, so we can do this in two compiler invocations:

  rustc --emit dep-info -o .deps/foo.a.pp ...
  rustc --emit link -o foo.a ...

This solution is not great, because we're parsing all the Rust code
twice.  It's also not great for two other reasons:

- rustc continues to create .deps/foo.d, not .deps/foo.a.pp as one might
  expect.

- The depfile rustc creates looks like:

  .deps/foo.a.pp: ...

  which has nothing to do with the object that we're creating.  And we
  couldn't even include this file for specifying dependencies, because
  the dependencies are wrong.

We could try to use -o .deps/foo.a, but that will still generate a .d
file (not a .pp, as we desire), and the object specified in the
generated depfile still won't be correct.

So -o is not fine-grained enough control for depfiles, at least with
rustc's current capabilities.  (gcc has the -MF option for fine-grained
control of where the depfile gets placed, while -o serves to denote
where the actual compiled thing goes; clang has the same thing and I think we
have some gross hack for MSVC to accomplish this sort of thing.)

I don't know what the right solution is here.  Couple of options from my
perspective:

1. Enhance rustc to have a depfile model closer to our current set of
   compilers.  I have no idea what sort of things this might break.

   It would be nice to teach rustc to not complain about emitting
   dep-info and objects at the same time, at least...

2. Suck it up and accept that depfiles for rust sources are just going
   to be a little weird.  Insert special cases in rules.mk for including
   such depfiles for $(RSOBJS) and accept that objdirs are going to get
   a little cluttered.

3. Something else (move depfiles to the expected place?).

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8666875 [details] [diff] [review]
attempt to have rustc emit proper dependency info for rust libraries

f? to Alex to look over the Rust side of things from comment 3.

Comment 5

[Brian Anderson]

From my perspective we might as well understand exactly what gecko wants and design a clean new (unstable) interface to rustc that does what gecko needs (other's are going to want it too).

Comment 6

[Nathan Froyd [:froydnj]]

For depfile generation on Linux, Gecko uses:

  g++ -MD -MP -MF $(depfile) ...

-MD is roughly equivalent to --emit dep-info.  -MP indicates that dummy targets for #include'd files should be added, which I don't think applies in the Rust case.  -MF is the missing piece here, which specifies the name of the depfile.  So:

  rustc --emit dep-info -o foo.a

would work as it does today, generating foo.d, containing a rule for foo.a.

  rustc --emit dep-info -o foo.a --dep-file .deps/foo.a.pp

would generate the depfile .deps/foo.a.pp, containing a rule for foo.a.  --dep-file without a corresponding -o option would be an error, I think. And ideally:

  rustc --emit dep-info,link -o foo.a --dep-file .deps/foo.a.pp

would generate .deps/foo.a.pp, containing a rule for foo.a, and foo.a, all without warning about specifying multiple things for --emit.  mshal, am I missing anything?

Comment 7

[Alex Crichton [:acrichto]]

Oh dear, sounds like there's definitely a few corner cases in dep-info inside rustc which may want to be ironed out! I agree with Brian that if we want to add new flags or fix functionality it's probably best to start from scratch rather than trying to bolt too many solutions onto the existing dep-info.

> causes rustc to complain that it's going to ignore -o, since we're
> asking it to output multiple things.  

This is currently because the compiler treats -o as "the one true output file" which isn't possible in this case because it's emitting two files (and only has one filename to put it into). One possible fix here is to use --out-dir to specify the root location for all output, and another would be to have the compiler infer the output name for each output type (e.g. adding .d or .pp to dep-info files)

> - rustc continues to create .deps/foo.d, not .deps/foo.a.pp as one might
>   expect.

This definitely sounds like a bug!

> - The depfile rustc creates looks like:
> 
>   .deps/foo.a.pp: ...

Yeah this is unfortunately where things get a little hairy. The compiler in theory wants to emit a directive for the output file being produced, but in this case the only output file is the dep-info file itself. The compiler doesn't otherwise know if it's producing an rlib, dylib, staticlib, exe, etc. It sounds to me like a new flag of some form is warranted here!
 

> 1. Enhance rustc to have a depfile model closer to our current set of
>    compilers.  I have no idea what sort of things this might break.

I think this is probably the best way forward (on our end at least).

> 3. Something else (move depfiles to the expected place?).

This may be the best interim solution for "getting something done" quickly at least

Comment 8

[Alex Crichton [:acrichto]]

I've opened an issue [1] on the Rust repo to track this from our side as well. Thanks for the feedback!

[1]: https://github.com/rust-lang/rust/issues/28716

Comment 9

[Mike Hommey [:glandium]]

> -MP indicates that dummy targets for #include'd files should be added,
> which I don't think applies in the Rust case.

Dummy targets are always needed. They prevent make failing because of removed files.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #9)
> > -MP indicates that dummy targets for #include'd files should be added,
> > which I don't think applies in the Rust case.
> 
> Dummy targets are always needed. They prevent make failing because of
> removed files.

Well, that's a separate issue, then, and can probably be solved reasonably easy...I guess we want a separate option for that?  I'll open a rust issue tonight or tomorrow unless Alex beats me to it.

Comment 11

[Gregory Szorc [:gps]]

In rare (but discouraged) cases, we wrap process output or post process file output to get the desired behavior so things work with make and our build system. While there is precedent for this, it is highly preferred for tools to "just work" out of the box. If nothing else, it lowers the overhead of the build system.

Comment 12

[Michael Shal [:mshal]]

(In reply to Nathan Froyd [:froydnj] from comment #6)
>   rustc --emit dep-info,link -o foo.a --dep-file .deps/foo.a.pp
> 
> would generate .deps/foo.a.pp, containing a rule for foo.a, and foo.a, all
> without warning about specifying multiple things for --emit.  mshal, am I
> missing anything?

I think that sounds reasonable, other than highlighting #c9 (otherwise you'll need clobbers for removing files).

As a stop-gap until rustc supports that, just special-casing rust files in MDDEPEND_FILES or doing a && 'mv foo.d .deps/foo.pp' should both work. The latter might be better so there aren't duplicate foo.d / foo.pp files in the objdir if/when we switch to a rustc with --dep-file. (It's not a huge deal either way, but may be confusing for people who dig into the objdir).

Comment 13

[Michael Shal [:mshal]]

Comment on attachment 8666875 [details] [diff] [review]
attempt to have rustc emit proper dependency info for rust libraries

From your comments it sounds like you need to use '-o $(call mk_libname,$<)' in both rustc invocations, and avoid telling rustc about MDDEPDIR. Otherwise you have to use sed or something to fixup the target in the .d file, right? I think something like this should work:

$(RUSTC) $(RUSTFLAGS) --crate-type staticlib --emit dep-info -o $(call mk_libname,$<) $(_VPATH_SRCS) && mv $<.d $(MDDEPDIR)/$<.pp

(I'm not sure if that mv command is correct, but hopefully that makes sense).

What happens if you omit the -o flag in the single-line 'rustc --emit dep-info,link' version? Do we get both of the expected files (foo.a and foo.d) without the error/warning? If so, doing that with an mv to the expected dep location might be simplest for now.

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8666875 [details] [diff] [review]
attempt to have rustc emit proper dependency info for rust libraries

Looks like this is being addressed on the rust side in https://github.com/rust-lang/rust/pull/28768, so we should just wait for Rust 1.4 or so to be released before fixing this.

Comment 15

[Alex Crichton [:acrichto]]

Ah so actually that PR, when merged, will be slated for 1.5 instead of 1.4 (as 1.4 is in beta right now). If it's urgent, however, to push it forward we can consider backporting it to 1.4 (as it's relatively minor). In any case it'll be in 1.5 beta (released when 1.4 stable is released), so it should be coming soon!

Comment 16

[Nathan Froyd [:froydnj]]

Attachment: part 1 - require Rust 1.5 for --enable-rust builds

Rust 1.5 features fine-grained dependency tracking that we need for
proper incremental builds.  As a side effect, it also features choosing
between the builtin-to-Rust jemalloc and the system allocator, which
Gecko needs for interoperation.  (Gecko needs to make Rust use the
system allocator, which then gets redirected into Gecko's copy of
jemalloc if necessary.)  It's not great to require an unstable Rust
target here, but we need to upgrade at some point anyway, so we might as
well make people aware of the upgrade requirement sooner rather than
later.

Comment 17

[Nathan Froyd [:froydnj]]

Attachment: emit proper dependencies when compiling Rust sources

Comment 18

[Ralph Giles (:rillian)]

Comment on attachment 8669785 [details] [diff] [review]
part 1 - require Rust 1.5 for --enable-rust builds

Review of attachment 8669785 [details] [diff] [review]:
-----------------------------------------------------------------

Looks correct. NB bug 1205012 will bit-rot this. Perhaps mshal can approve both so we can land them together.

I'd rather see us wait for the rustc change to be released before landing this.

Comment 19

[Michael Shal [:mshal]]

Comment on attachment 8669793 [details] [diff] [review]
emit proper dependencies when compiling Rust sources

LGTM :)

Comment 20

[Ralph Giles (:rillian)]

Attachment: part 1 - require Rust 1.5 for --enable-rust builds v2

Rebase rust minversion bump on top of bug 1205012. Carrying forward r=mshal

Comment 21

[Ralph Giles (:rillian)]

Attachment: Require Rust 1.5 for --enable-rust builds v3

Rebase on top of 1.4 bump. Carrying r=mshal.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3fd530324ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5618a5cbc7f

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This (or bug 1211562, also backed out) broke mulet builds like https://treeherder.mozilla.org/logviewer.html#?job_id=18719145&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/46abb0f41d94

[1G[J[34m 0:27.32(B[m configure: error: Rust compiler 1.4.0-dev is too old.
[1G[J[34m 0:27.32(B[m       To compile Rust language sources please install at least
[1G[J[34m 0:27.32(B[m       version 1.5 of the 'rustc' toolchain and make sure it is
[1G[J[34m 0:27.32(B[m       first in your path.
[1G[J[34m 0:27.32(B[m       You can verify this by typing 'rustc --version'.
[1G[J[34m 0:27.32(B[m ------ config.log ------
[1G[J[34m 0:27.33(B[m configure:3715: checking for c++
[1G[J[34m 0:27.33(B[m configure:3747: checking whether the C++ compiler (/usr/bin/ccache /home/worker/workspace/gecko/./gcc/bin/g++  -L/home/worker/workspace/gecko/gtk3/usr/local/lib ) works
[1G[J[34m 0:27.33(B[m configure:3763: /usr/bin/ccache /home/worker/workspace/gecko/./gcc/bin/g++ -o conftest   -L/home/worker/workspace/gecko/gtk3/usr/local/lib  conftest.C  1>&5
[1G[J[34m 0:27.33(B[m configure:3789: checking whether the C++ compiler (/usr/bin/ccache /home/worker/workspace/gecko/./gcc/bin/g++  -L/home/worker/workspace/gecko/gtk3/usr/local/lib ) is a cross-compiler
[1G[J[34m 0:27.33(B[m configure:3794: checking whether we are using GNU C++
[1G[J[34m 0:27.33(B[m configure:3822: checking whether /usr/bin/ccache /home/worker/workspace/gecko/./gcc/bin/g++ accepts -g
[1G[J[34m 0:27.33(B[m configure:3871: /usr/bin/ccache /home/worker/workspace/gecko/./gcc/bin/gcc -c   conftest.c 1>&5
[1G[J[34m 0:27.33(B[m configure:3888: /usr/bin/ccache /home/worker/workspace/gecko/./gcc/bin/gcc -c   conftest.c 1>&5
[1G[J[34m 0:27.33(B[m configure: In function 'main':
[1G[J[34m 0:27.33(B[m Warning: enabled by default in configure: incompatible implicit declaration of built-in function 'exit'
[1G[J[34m 0:27.33(B[m configure:3884:1: warning: incompatible implicit declaration of built-in function 'exit' [enabled by default]
[1G[J[34m 0:27.33(B[m configure:3907: checking for ranlib
[1G[J[34m 0:27.33(B[m configure:3939: checking for as
[1G[J[34m 0:27.33(B[m configure:3993: checking for ar
[1G[J[34m 0:27.33(B[m configure:4028: checking for ld
[1G[J[34m 0:27.33(B[m configure:4063: checking for strip
[1G[J[34m 0:27.33(B[m configure:4098: checking for windres
[1G[J[34m 0:27.33(B[m configure:4133: checking for otool
[1G[J[34m 0:27.33(B[m configure:4342: checking for ccache
[1G[J[34m 0:27.33(B[m configure:4436: checking for rustc
[1G[J[34m 0:27.33(B[m configure:4483: checking rustc version
[1G[J[34m 0:27.33(B[m configure: error: Rust compiler 1.4.0-dev is too old.
[1G[J[34m 0:27.33(B[m       To compile Rust language sources please install at least
[1G[J[34m 0:27.33(B[m       version 1.5 of the 'rustc' toolchain and make sure it is
[1G[J[34m 0:27.33(B[m       first in your path.
[1G[J[34m 0:27.33(B[m       You can verify this by typing 'rustc --version'.
[1G[J[34m 0:27.33(B[m *** Fix above errors and then restart with\
[1G[J[34m 0:27.33(B[m                "/usr/bin/gmake -f client.mk build"
[1G[J[34m 0:27.33(B[m gmake[2]: *** [configure] Error 1
[1G[J[34m 0:27.33(B[m gmake[1]: *** [/home/worker/workspace/object-folder/config.status] Error 2
[1G[J[34m 0:27.33(B[m gmake: *** [build] Error 2
[1G[J[34m 0:27.35(B[m 468 compiler warnings present.
[1G[J[34m 0:28.52(B[m ccache (direct) hit rate: 0.0%; (preprocessed) hit rate: 0.0%; miss rate: 100.0%

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7dc34520110
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5e87026ac8

Comment 25

[Nathan Froyd [:froydnj]]

Thanks for taking care of this, Ralph!

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c7dc34520110
https://hg.mozilla.org/mozilla-central/rev/2a5e87026ac8