Closed Bug 1381955 Opened 3 years ago Closed 3 years ago

Respect RUSTFLAGS after Bug 1380381

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: tomprince, Assigned: tomprince)

References

Details

Attachments

(1 file)

Bug 1380381 inadvertently overrides RUSTFLAGS everywhere to just specify the debuginfo level.

Since Thunderbird needs supply a debuginfo for the same reason as that bug for native OSX builds, we need support passing RUSTFLAGS from mozconfig again.
Blocks: 1380844
In fact, this is worse than that, bug 1380381 also overrides the flags set in config/rules.mk itself, for e.g. non-optimized builds.

Mike, can you fix this?
Blocks: 1380381
Flags: needinfo?(mshal)
> Mike, can you fix this?

There is already a fix, it just needs someone to review and land it.
Comment on attachment 8887623 [details]
Bug 1381955 - Don't override RUSTFLAGS to set rust's debuginfo;

https://reviewboard.mozilla.org/r/158500/#review163954

::: config/rules.mk:911
(Diff revision 1)
>  sccache_wrap := RUSTC_WRAPPER='$(CCACHE)'
>  endif
>  
>  # XXX hack to work around dsymutil failing on cross-OSX builds (bug 1380381)
>  ifeq ($(HOST_OS_ARCH)-$(OS_ARCH),Linux-Darwin)
> -rust_debug_info=1
> +default_rustflags += -C debuginfo=1

We still want the -C debuginfo=2 for the `else` case.
Flags: needinfo?(mshal)
Attachment #8887623 - Flags: review?(ted)
Comment on attachment 8887623 [details]
Bug 1381955 - Don't override RUSTFLAGS to set rust's debuginfo;

https://reviewboard.mozilla.org/r/158500/#review163954

> We still want the -C debuginfo=2 for the `else` case.

Is that actually needed? Looking at [the previous patch](https://reviewboard.mozilla.org/r/157208/diff/2#index_header), neither was set before. I had assumed that the `debuginfo=2` was added because `debuginfo` was being passed unconditionally.
(In reply to Tom Prince [:tomprince] from comment #6)
> Comment on attachment 8887623 [details]
> Bug 1381955 - Don't override RUSTFLAGS to set rust's debuginfo;
> 
> https://reviewboard.mozilla.org/r/158500/#review163954
> 
> > We still want the -C debuginfo=2 for the `else` case.
> 
> Is that actually needed? Looking at [the previous
> patch](https://reviewboard.mozilla.org/r/157208/diff/2#index_header),
> neither was set before. I had assumed that the `debuginfo=2` was added
> because `debuginfo` was being passed unconditionally.

That patch also removed debug=true from the cargo build profile, which means cargo won't set it itself. Which is why it's needed.
Comment on attachment 8887623 [details]
Bug 1381955 - Don't override RUSTFLAGS to set rust's debuginfo;

https://reviewboard.mozilla.org/r/158500/#review163958
Attachment #8887623 - Flags: review+
Keywords: checkin-needed
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/405a90abd28a
Don't override RUSTFLAGS to set rust's debuginfo; r=glandium
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/405a90abd28a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Attachment #8887623 - Flags: review?(ted)
Product: Core → Firefox Build System
Assignee: nobody → mozilla
You need to log in before you can comment on or make changes to this bug.