Change platform identifier for ASan builds

RESOLVED FIXED in Firefox 60

Status

enhancement
--
major
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: decoder, Assigned: decoder)

Tracking

({sec-want})

Trunk
mozilla60
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(firefox59 wontfix, firefox60 fixed)

Details

(Whiteboard: [adv-main60-])

Attachments

(2 attachments)

In bug 1403548, we are trying to get updates for the ASan Nightly Reporter builds. In order to make that possible, RelEng wants us to change the platform identifier for these builds.

The attached patch does that for all ASan builds because I didn't find a way to only do this for the builds that have the reporter enabled. According to RelEng that shouldn't be a problem though.

Mike, can you please let me know if the upcoming patch does this in the right way? I don't know where this is changed best, so if you have better ideas, please let me know :)
Comment on attachment 8940221 [details]
Bug 1428357 - Change ABI used in app update URL for ASan builds.

https://reviewboard.mozilla.org/r/210530/#review219188

This doesn't seem like the right place to do this. Can't you use a separate update channel?
Attachment #8940221 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #2)
> Comment on attachment 8940221 [details]
> Bug 1428357 - Add a suffix to TARGET_COMPILER_ABI for ASan builds.
> 
> https://reviewboard.mozilla.org/r/210530/#review219188
> 
> This doesn't seem like the right place to do this. Can't you use a separate
> update channel?

No and RelEng explicitly requested that I should change the platform identifier so we can mask the ASan builds as a separate architecture rather than a separate channel.
Flags: needinfo?(mh+mozilla)
Why can't a separate channel be used?
Flags: needinfo?(mh+mozilla) → needinfo?(catlee)
It's possible to use a separate channel, but it requires breaking a lot of assumptions about "nightly".

What are your concerns with changing the platform identifier?
Flags: needinfo?(catlee)
Chris, if we go for the platform identifier solution, does the platform identifier itself have to be changed (like in the patch here) or does it suffice if the build just uses a different string when pinging the update server? If the latter is sufficient, we could make the change in the code that talks to the update server and leave the actual ABI untouched.
Flags: needinfo?(catlee)
I think that changing the string used to query the update server is sufficient.
Flags: needinfo?(catlee)
(In reply to Christian Holler (:decoder) from comment #8)
> Comment on attachment 8940221 [details]
> Bug 1428357 - Change ABI used in app update URL for ASan builds.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/210530/diff/1-2/


This changes the ABI string for ASan builds as suggested by catlee (see previous comments).
Comment on attachment 8940221 [details]
Bug 1428357 - Change ABI used in app update URL for ASan builds.

https://reviewboard.mozilla.org/r/210530/#review221122

Looks fine. There are xpcshell tests for this that I suspect will fail on asan with this change. Please check and submit a separate test patch if needed.
Attachment #8940221 - Flags: review?(robert.strong.bugs) → review+
Comment on attachment 8940221 [details]
Bug 1428357 - Change ABI used in app update URL for ASan builds.

https://reviewboard.mozilla.org/r/210530/#review221242

We're going to need to change the balrog submission code as well (otherwise ASan will stomp on non-ASan nightlies). This will involve at least a change to to https://github.com/mozilla/build-tools/blob/master/lib/python/release/platforms.py#L22, to map the -asan platforms to the new ABIs. I think it's also going to require some task graph work to get beetmover, balrog submission, and possibly other things added to the graph.
Attachment #8940221 - Flags: review?(bhearsum) → review+
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #10)
> Comment on attachment 8940221 [details]

> Looks fine. There are xpcshell tests for this that I suspect will fail on
> asan with this change. Please check and submit a separate test patch if
> needed.

Thanks for pointing this out. I found one xpcshell test that failed in try with the patch and fixed that failure in the second patch I just pushed to review.
Comment on attachment 8946319 [details]
Bug 1428357 - Fix UpdateUtils test to reflect ASan ABI change.

https://reviewboard.mozilla.org/r/216298/#review222058
Attachment #8946319 - Flags: review?(robert.strong.bugs) → review+
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4229e878ca74
Change ABI used in app update URL for ASan builds. r=bhearsum,rstrong
https://hg.mozilla.org/integration/autoland/rev/0777a1e9ff32
Fix UpdateUtils test to reflect ASan ABI change. r=rstrong
https://hg.mozilla.org/mozilla-central/rev/4229e878ca74
https://hg.mozilla.org/mozilla-central/rev/0777a1e9ff32
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Product: Core → Firefox Build System
Whiteboard: [adv-main60-]
You need to log in before you can comment on or make changes to this bug.