1430678 - Add CI check for release builds to verify against compiling in debug impls

Status: RESOLVED WONTFIX

(Firefox Build System :: General, enhancement)

Description

[Bobby Holley (:bholley)]

In bug 1351737 comment 33, I'm removing usage of rust Debug trait implementations in release builds to reduce codesize. The wins are big, but I'm concerned that we'll regress them, because it's really easy to do so by accident.

It seems like it would be straightforward to hook up something on automation, potentially as part of |make check|, that does nm on release builds and greps the output for core..fmt..Debug. We could compare the result against a whitelist in case somebody really did want to pull in certain implementations in.

Comment 1

[Bobby Holley (:bholley)]

Nathan, does this seem straightforward to do?

Comment 2

[Mike Hommey [:glandium]]

Shouldn't you do that on rust end instead (and make it a problem on servo automation, rather than wait for the failure to happen on firefox CI)?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Mike Hommey [:glandium] from comment #2)
> Shouldn't you do that on rust end instead

Compiler support would certainly be ideal, but that seems like a steeper mountain to climb than what I'm proposing. Unless you really just meant "servo" here too?

> (and make it a problem on servo
> automation, rather than wait for the failure to happen on firefox CI)?

Catching this there would be good too, but wouldn't catch all the other rust usage in Gecko, and there appears to be 70k of Debug impls left in libxul even after removing all the ones from stylo (looking into this is on my todo list).

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) from comment #1)
> Nathan, does this seem straightforward to do?

This is super-straightforward, and we already do a number of checks like this; see CHECK_SYMBOLS in config/config.mk and its related uses in that file.

I'd echo glandium's concerns.  My interpretation was that he meant to check things on the servo side, rather than doing anything with rustc, though I think Xidorn's suggestion on dev-platform was actually implementing a new switch on the rustc side?  A rustc-side fix seems pretty narrow to me, and better accomplished elsewhere.

It's not clear to me that servo necessarily wants to follow the same rules about Debug impls, though, even if we think it'd be good to do so.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> (In reply to Bobby Holley (:bholley) from comment #1)
> > Nathan, does this seem straightforward to do?
> 
> This is super-straightforward, and we already do a number of checks like
> this; see CHECK_SYMBOLS in config/config.mk and its related uses in that
> file.

Great!

> I'd echo glandium's concerns.  My interpretation was that he meant to check
> things on the servo side, rather than doing anything with rustc, though I
> think Xidorn's suggestion on dev-platform was actually implementing a new
> switch on the rustc side?  A rustc-side fix seems pretty narrow to me, and
> better accomplished elsewhere.
> 
> It's not clear to me that servo necessarily wants to follow the same rules
> about Debug impls, though, even if we think it'd be good to do so.

I don't think there was any disagreement on the thread so far (Anthony retracted his). In general the Servo team is willing to prioritize product requirements for shared code.

I don't follow the reasoning for doing this on the servo side, since per comment 3 I think we want to check all the Rust in Gecko, not just the Stylo code. I agree it would be nice to catch mistakes earlier (before they sync over), but the sheriffs can always whitelist and file a bug.

Can you clarify whether there are any other pieces I'm missing above? If not, is this something you (or someone else on the LLTT) can take care of after I eliminate the remaining 70k of Debug impls in Gecko?

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) from comment #5)
> I don't follow the reasoning for doing this on the servo side, since per
> comment 3 I think we want to check all the Rust in Gecko, not just the Stylo
> code. I agree it would be nice to catch mistakes earlier (before they sync
> over), but the sheriffs can always whitelist and file a bug.

Checking all the Rust in Gecko would indeed be ideal.  We'll see whether this causes pain with third-party deps down the road, I guess.

> Can you clarify whether there are any other pieces I'm missing above? If
> not, is this something you (or someone else on the LLTT) can take care of
> after I eliminate the remaining 70k of Debug impls in Gecko?

Sure.  Can you file a bug, assign to me, and have anything you file on the Gecko side block that bug?  Thanks!

Comment 7

[Bobby Holley (:bholley)]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> (In reply to Bobby Holley (:bholley) from comment #5)
> > I don't follow the reasoning for doing this on the servo side, since per
> > comment 3 I think we want to check all the Rust in Gecko, not just the Stylo
> > code. I agree it would be nice to catch mistakes earlier (before they sync
> > over), but the sheriffs can always whitelist and file a bug.
> 
> Checking all the Rust in Gecko would indeed be ideal.  We'll see whether
> this causes pain with third-party deps down the road, I guess.

Yeah, I think a whitelist there will be crucial for working around code we don't control, at least temporarily.

> 
> > Can you clarify whether there are any other pieces I'm missing above? If
> > not, is this something you (or someone else on the LLTT) can take care of
> > after I eliminate the remaining 70k of Debug impls in Gecko?
> 
> Sure.  Can you file a bug, assign to me

This bug will do, assigned to you.

> and have anything you file on the Gecko side block that bug?  Thanks!

Filed bug 1430814. It's on my list.

Comment 8

[Bobby Holley (:bholley)]

Per bug 1430814 comment 1 I think this isn't realistic. We'll have to rely on code review and periodic audits, and accept some slop. :-(