1318981 - Pass DIST as environment variable to cargo

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

As discussed before, if we want the build script to generate bindings on build-time, we need to inform it about the object directory. It should be passed into cargo via MOZ_OBJDIR environment variable.

I think we need the absolute path of that value.

It seems we invoke cargo in config/rules.mk, but I don't see MOZ_OBJDIR anywhere in that file... So I do not immediately have idea how to add it...

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

froydnj, IIRC you said it should be straightforward to add. Could you take a look at this?

Comment 2

[Mike Hommey [:glandium]]

cargo is already building things in the objdir, so why would binding generation need to know MOZ_OBJDIR specifically? It should just output its stuff wherever cargo is doing the build.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Build-time bindgen needs to read headers from $MOZ_OBJDIR/dist/include.

Comment 4

[Mike Hommey [:glandium]]

So you want DIST, not MOZ_OBJDIR.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

That may work as well. It seems we probably also need mozilla-config.h, which is not inside DIST.

Comment 6

[Mike Hommey [:glandium]]

It is.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #1)
> froydnj, IIRC you said it should be straightforward to add. Could you take a
> look at this?

Sure.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Mike Hommey [:glandium] from comment #6)
> It is.

Oh, so there is an identical mozilla-config.h in dist/include. Ok, then DIST is probably fine as well. That isn't currently provided to cargo either, so I guess there is still some work need to be done.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 1 - factor out a CARGO_BUILD makefile variable

This change makes the individual cargo rules easier to understand, and
provides a single place to change e.g. environment variables passed to
cargo.

This change applies on top of bug 1293253, which will be landing shortly.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 2 - pass DIST as an environment variable to `cargo build`

rust-bindgen, at least, will need to know where Gecko's headers are to
parse them.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Could we make the name of the env var more specific? Probably MOZ_DIST?

Comment 12

[Mike Hommey [:glandium]]

I don't know what the build.rs using rust-bindgen does, but if the vendored crate is not mozilla specific, it seems to me you don't want a mozilla-specific variable, but a generic one, that says where headers to create bindings for are.

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: part 1 - factor out a CARGO_BUILD makefile variable

Let's provide the right variables in the modified rules.

Comment 14

[Mike Hommey [:glandium]]

(incidentally, DIST wouldn't be very good for that either)

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Mike Hommey [:glandium] from comment #12)
> I don't know what the build.rs using rust-bindgen does, but if the vendored
> crate is not mozilla specific, it seems to me you don't want a
> mozilla-specific variable, but a generic one, that says where headers to
> create bindings for are.

build.rs is part of servo/style component, which would include lots of mozilla-specific things, like which headers would be used to generate which module, which types would be generated from headers. So that is something specific to mozilla.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Could we change to use MOZ_DIST or something like that instead of just DIST? I guess DIST is probably also fine as it doesn't seem to conflict with anything else at this moment, though I prefer something more mozilla-specific here.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #16)
> Could we change to use MOZ_DIST or something like that instead of just DIST?
> I guess DIST is probably also fine as it doesn't seem to conflict with
> anything else at this moment, though I prefer something more
> mozilla-specific here.

Sure, I can do that.  I was expecting ted to r+ with commentary on whether MOZ_DIST or DIST is preferred, but I can do the change first since ted appears to be taking a while. :p

Comment 18

[Nathan Froyd [:froydnj]]

Attachment: part 2 - pass MOZ_DIST as an environment variable to `cargo build`

Changed to MOZ_DIST, as Xidorn requests.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f49a4f2953f3
part 1 - factor out a CARGO_BUILD makefile variable; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9d80b5b44fc
part 2 - pass MOZ_DIST as an environment variable to `cargo build`; r=chmanchester

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f49a4f2953f3
https://hg.mozilla.org/mozilla-central/rev/d9d80b5b44fc

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

It seems MOZ_DIST is not an absolute path. Relative path would not work as the build script is run in some... unrelated path. froydnj, could you fix that?

Comment 22

[Xidorn Quan [:xidorn] UTC+11]

Had a patch to fix that.