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

Add --dep-info flag to write Makefile compatible dependency files #10698

Merged
merged 3 commits into from Dec 13, 2013

Conversation

Projects
None yet
5 participants
@metajack
Copy link
Contributor

metajack commented Nov 28, 2013

This isn't super useful for libraries yet without #10593.

Fixes #7633.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Nov 28, 2013

Out of curiosity, why are we reviving this issue? Or more specifically, why is the rustpkg solution not being used?

@metajack

This comment has been minimized.

Copy link
Contributor

metajack commented Nov 28, 2013

It doesn't look like rustpkg will be ready to handle all of Servo's needs in the near future and I want more reliable builds. With a few small changes (this + #10593) external build tooling becomes much easier to use.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Nov 28, 2013

It sounds like this should maybe be a -Z flag becuase this won't be supported for forever? (nor is it the recommended way of doing things).

@metajack

This comment has been minimized.

Copy link
Contributor

metajack commented Nov 28, 2013

Why wouldn't we support it? There will always be reasons to use external build tooling, such as when non-Rust projects take Rust libraries as dependencies.

@pcwalton

This comment has been minimized.

Copy link
Contributor

pcwalton commented Nov 28, 2013

I agree with @metajack. This is a useful flag that we should support.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Nov 28, 2013

This should also have a test, just to make sure that nothing dies along this path.

@metajack

This comment has been minimized.

Copy link
Contributor

metajack commented Dec 11, 2013

Rebased. Need to add a test still.

@metajack

This comment has been minimized.

Copy link
Contributor

metajack commented Dec 11, 2013

@alexcrichton r? test added. The rest was previously reviewed by @pcwalton although it had a minor interaction with your patches during rebase, so you might take a glace at that.

@metajack

This comment has been minimized.

Copy link
Contributor

metajack commented Dec 12, 2013

@alexcrichton I addressed your comments in two new commits. I'll squash it down after you review.

bors added a commit that referenced this pull request Dec 12, 2013

auto merge of #10698 : metajack/rust/dep-info, r=alexcrichton
This isn't super useful for libraries yet without #10593.

Fixes #7633.
Add --dep-info to write Makefile-compatible dependency info.
When --dep-info is given, rustc will write out a `$input_base.d` file in the
output directory that contains Makefile compatible dependency information for
use with tools like make and ninja.
@metajack

This comment has been minimized.

Copy link
Contributor

metajack commented Dec 12, 2013

Fixed rustpkg's call to phase_6. Otherwise nothing changed.

@metajack

This comment has been minimized.

Copy link
Owner

metajack commented on b2ccd4c Dec 13, 2013

r=alexcrichton

This comment has been minimized.

Copy link

alexcrichton replied Dec 13, 2013

@bors: retry

This comment has been minimized.

Copy link
Owner

metajack replied Dec 13, 2013

@bors: retry

@metajack

This comment has been minimized.

Copy link
Contributor

metajack commented Dec 13, 2013

Added some minor test fixes to account for TMPDIR.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented on b2ccd4c Dec 13, 2013

saw approval from alexcrichton
at metajack@b2ccd4c

This comment has been minimized.

Copy link
Contributor

bors replied Dec 13, 2013

merging metajack/rust/dep-info = b2ccd4c into auto

This comment has been minimized.

Copy link
Contributor

bors replied Dec 13, 2013

metajack/rust/dep-info = b2ccd4c merged ok, testing candidate = 15fd0c0d

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

bors replied Dec 13, 2013

saw approval from alexcrichton
at metajack@b2ccd4c

This comment has been minimized.

Copy link
Contributor

bors replied Dec 13, 2013

saw approval from alexcrichton
at metajack@b2ccd4c

This comment has been minimized.

Copy link
Contributor

bors replied Dec 13, 2013

merging metajack/rust/dep-info = b2ccd4c into auto

This comment has been minimized.

Copy link
Contributor

bors replied Dec 13, 2013

metajack/rust/dep-info = b2ccd4c merged ok, testing candidate = 9bbef13

This comment has been minimized.

Copy link
Contributor

bors replied Dec 13, 2013

fast-forwarding master to auto = 9bbef13

bors added a commit that referenced this pull request Dec 13, 2013

auto merge of #10698 : metajack/rust/dep-info, r=alexcrichton
This isn't super useful for libraries yet without #10593.

Fixes #7633.

@bors bors merged commit b2ccd4c into rust-lang:master Dec 13, 2013

1 check passed

default all tests passed
@zargony

This comment has been minimized.

Copy link
Contributor

zargony commented Dec 15, 2013

This is cool. Makefiles are much easier now. Great work!

However, I found a slight problem. When calling rustc twice with the same crate source file, the second compile overwrites the dependency file generated by the first compile. This happens to me when compiling tests from the same source file as the library.
Because the deps file name is based on the source file name, rustc --dep-info --test lib.rs will overwrite the deps file from rustc --dep-info --dylib --rlib lib.rs.
Wouldn't it be more practical to use the target file name as the base for the deps file name? (Or maybe optionally passing a file name as command line parameter)

@metajack

This comment has been minimized.

Copy link
Contributor

metajack commented Dec 16, 2013

gcc does indeed have options for specifying the filename, but I didn't use that part, instead implementing the -MMD version which does similar inference as this.

A workaround (and the recommended way I think) is to have tests start from test.rs.

You might file a bug to add the ability for --dep-info to optionally take an output file name. That shouldn't be difficult to add.

@metajack metajack deleted the metajack:dep-info branch Dec 16, 2013

@zargony

This comment has been minimized.

Copy link
Contributor

zargony commented Dec 17, 2013

What do you think about using the target filename with extension replaced by ".d" for the deps file? That sounds more practical to me since you could build more than one target from a source (e.g. with different --cfg settings, or library and test like I tried. I know the recommended way is to build tests from test.rs, but especially for small projects, using the same source file for tests can make things easier).

@metajack

This comment has been minimized.

Copy link
Contributor

metajack commented Dec 17, 2013

That sounds fine to me. Want to submit a PR? :)

@zargony

This comment has been minimized.

Copy link
Contributor

zargony commented Dec 18, 2013

Sure, I'll look into it. May take me a little time since I'm mostly familiar with libstd/libextra and not librustc yet, but changing the name shouldn't be too hard :)

bors added a commit that referenced this pull request Dec 21, 2013

auto merge of #11046 : zargony/rust/dep-info, r=alexcrichton
Using --dep-info writes Makefile-compatible dependency info to a file that is by default named based on the crate source filename. This PR adds an optional string argument to the --dep-info option which allows to write dependency info to an arbitrary filename.

cc #10698
cc @metajack

bors added a commit that referenced this pull request Dec 22, 2013

auto merge of #11046 : zargony/rust/dep-info, r=alexcrichton
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment