Closed
Bug 1318981
Opened 6 years ago
Closed 6 years ago
Pass DIST as environment variable to cargo
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox53 fixed)
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: xidorn, Assigned: froydnj)
References
Details
Attachments
(2 files, 2 obsolete files)
2.38 KB,
patch
|
chmanchester
:
review+
|
Details | Diff | Splinter Review |
1.26 KB,
patch
|
chmanchester
:
review+
|
Details | Diff | Splinter Review |
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...
Reporter | ||
Comment 1•6 years ago
|
||
froydnj, IIRC you said it should be straightforward to add. Could you take a look at this?
Flags: needinfo?(nfroyd)
Comment 2•6 years ago
|
||
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.
Reporter | ||
Comment 3•6 years ago
|
||
Build-time bindgen needs to read headers from $MOZ_OBJDIR/dist/include.
Comment 4•6 years ago
|
||
So you want DIST, not MOZ_OBJDIR.
Reporter | ||
Comment 5•6 years ago
|
||
That may work as well. It seems we probably also need mozilla-config.h, which is not inside DIST.
Comment 6•6 years ago
|
||
It is.
![]() |
Assignee | |
Comment 7•6 years ago
|
||
(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)
Reporter | ||
Comment 8•6 years ago
|
||
(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.
![]() |
Assignee | |
Updated•6 years ago
|
Summary: Pass MOZ_OBJDIR as environment variable to cargo → Pass DIST as environment variable to cargo
![]() |
Assignee | |
Comment 9•6 years ago
|
||
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)
![]() |
Assignee | |
Comment 10•6 years ago
|
||
rust-bindgen, at least, will need to know where Gecko's headers are to parse them.
Attachment #8813149 -
Flags: review?(ted)
Reporter | ||
Comment 11•6 years ago
|
||
Could we make the name of the env var more specific? Probably MOZ_DIST?
Comment 12•6 years ago
|
||
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.
![]() |
Assignee | |
Comment 13•6 years ago
|
||
Let's provide the right variables in the modified rules.
Attachment #8813162 -
Flags: review?(ted)
![]() |
Assignee | |
Updated•6 years ago
|
Attachment #8813148 -
Attachment is obsolete: true
Attachment #8813148 -
Flags: review?(ted)
Comment 14•6 years ago
|
||
(incidentally, DIST wouldn't be very good for that either)
Reporter | ||
Comment 15•6 years ago
|
||
(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.
Reporter | ||
Comment 16•6 years ago
|
||
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)
![]() |
Assignee | |
Comment 17•6 years ago
|
||
(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)
![]() |
Assignee | |
Comment 18•6 years ago
|
||
Changed to MOZ_DIST, as Xidorn requests.
Attachment #8815806 -
Flags: review?(ted)
![]() |
Assignee | |
Updated•6 years ago
|
Attachment #8813149 -
Attachment is obsolete: true
Attachment #8813149 -
Flags: review?(ted)
Updated•6 years ago
|
Attachment #8813162 -
Flags: review?(ted) → review+
Updated•6 years ago
|
Attachment #8815806 -
Flags: review?(ted) → review+
Comment 19•6 years ago
|
||
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
Comment 20•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f49a4f2953f3 https://hg.mozilla.org/mozilla-central/rev/d9d80b5b44fc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Reporter | ||
Comment 21•6 years ago
|
||
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)
Updated•5 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•