Pass DIST as environment variable to cargo

RESOLVED FIXED in Firefox 53

Status

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: xidorn, Assigned: froydnj)

Tracking

Trunk
mozilla53
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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...
froydnj, IIRC you said it should be straightforward to add. Could you take a look at this?
Flags: needinfo?(nfroyd)
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.
Build-time bindgen needs to read headers from $MOZ_OBJDIR/dist/include.
So you want DIST, not MOZ_OBJDIR.
That may work as well. It seems we probably also need mozilla-config.h, which is not inside DIST.
It is.
(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.
Assignee: nobody → nfroyd
Flags: needinfo?(nfroyd)
(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.
Summary: Pass MOZ_OBJDIR as environment variable to cargo → Pass DIST as environment variable to cargo
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.
Attachment #8813148 - Flags: review?(ted)
rust-bindgen, at least, will need to know where Gecko's headers are to
parse them.
Attachment #8813149 - Flags: review?(ted)
Could we make the name of the env var more specific? Probably MOZ_DIST?
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.
Let's provide the right variables in the modified rules.
Attachment #8813162 - Flags: review?(ted)
Attachment #8813148 - Attachment is obsolete: true
Attachment #8813148 - Flags: review?(ted)
(incidentally, DIST wouldn't be very good for that either)
(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.
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.
Flags: needinfo?(nfroyd)
(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
Flags: needinfo?(nfroyd)
Changed to MOZ_DIST, as Xidorn requests.
Attachment #8815806 - Flags: review?(ted)
Attachment #8813149 - Attachment is obsolete: true
Attachment #8813149 - Flags: review?(ted)
Attachment #8813162 - Flags: review?(ted) → review+
Attachment #8815806 - Flags: review?(ted) → review+
Pushed by nfroyd@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/f49a4f2953f3
https://hg.mozilla.org/mozilla-central/rev/d9d80b5b44fc
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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?
Flags: needinfo?(nfroyd)
Had a patch to fix that.
Flags: needinfo?(nfroyd)
Blocks: 1323127
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.