1253697 - |mach artifact install| downloads an opt build even with "ac_add_options --enable-debug" set in mozconfig

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

When I try to build a debug build with "ac_add_options --enable-debug" set in my mozconfig, artifact install downloads an opt build instead of a debug build.

https://queue.taskcluster.net/v1/task/HMR3odtuSJWzKoSNwF6mlw/artifacts/public%2Fbuild%2Ffirefox-47.0a1.en-US.linux-x86_64.tar.bz2

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I assume this would also apply to other kind of builds like ASAN etc. So maybe this should be more generic? Otherwise other bugs should be filed.

Comment 2

[Nick Alexander :nalexander [he/him]]

Follow-on to Bug 1243041.  Might be a good mentor bug.

Comment 3

[Nick Alexander :nalexander [he/him]]

Relevant code is at https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/artifacts.py#753.  We'd need to generalize 'linux' at https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/artifacts.py#396 to one of 'linux', 'linux-debug', since that appears to be how to choose between opt and debug.

Shouldn't be rocket science!

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: MozReview Request: Bug 1253697 - Support downloading debug artifact builds. r=nalexander

Review commit: https://reviewboard.mozilla.org/r/48059/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48059/

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Nick Alexander :nalexander from comment #3)
> Relevant code is at
> https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/
> artifacts.py#753.  We'd need to generalize 'linux' at
> https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/
> artifacts.py#396 to one of 'linux', 'linux-debug', since that appears to be
> how to choose between opt and debug.
> 
> Shouldn't be rocket science!

Hi Nick,

I wanted this feature so badly, that I could not resist to the temptation to prepare a draft patch for it ("no rocket science was harmed" during this patch preparation :-))

The current patch (Attachment 8743834 [details]) is probably far from perfect (it is the first time I look into the building system internals, and so I tried to find where the pieces should go, but I could easily be wrong).

Anyway this patch was sitting in my hg bookmarks for a while now, so I thought it is time to attaching it here, as the first step to make it a real thing.

NOTE: I've currently tested this patch only on linux (with the linux64 jobs), but theoretically it should work correctly on osx and windows too
(the current version doesn't cover android, because I wasn't sure if it follows the exactly same pattern of the other platforms)

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8743834 [details]
MozReview Request: Bug 1253697 - Support downloading debug artifact builds. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48059/diff/1-2/

Comment 7

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/48059/#review45583

Thanks for the patch!  However, we don't want to add new flags.  Rather than add a new flag, let's inspect `MOZ_DEBUG`.  See the example around https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/artifacts.py#748.

Thanks again!

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8743834 [details]
MozReview Request: Bug 1253697 - Support downloading debug artifact builds. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48059/diff/2-3/

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Nick Alexander :nalexander from comment #7)
> https://reviewboard.mozilla.org/r/48059/#review45583
> 
> Thanks for the patch!  However, we don't want to add new flags.  Rather than
> add a new flag, let's inspect `MOZ_DEBUG`.

Thanks Nick!
In the last version pushed to mozreview, I'm now checking the MOZ_DEBUG flag (and I removed from moz.configure the code which used to define the new options).

As a side note:
using "ac_add_options --enable-debug" doesn't seem to enable the MOZ_DEBUG flag if combined with "ac_add_options --enable-artifact-builds" (my guess is that it is due to "--enable-artifact-builds" which implies "--disable-compile-environment")

for the above reason this is the mozconfig file that I'm using with the last patch version to enable the debug artifact build:

    export MOZ_DEBUG=1
    ac_add_options --enable-artifact-builds

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8743834 [details]
MozReview Request: Bug 1253697 - Support downloading debug artifact builds. r=nalexander

https://reviewboard.mozilla.org/r/48059/#review45837

Looking better!  I have a few nits, and then we're good to go.

::: python/mozbuild/mozbuild/artifacts.py
(Diff revision 3)
> -        self._job = job or self._guess_artifact_job()
>          self._log = log
>          self._hg = hg
>          self._git = git
>          self._cache_dir = cache_dir
>          self._skip_cache = skip_cache
>  
> +        ## NOTE: _guest_artifact_job needs _log.
> +        self._job = job or self._guess_artifact_job()
> +

nit: Revert these changes; they're not needed.

::: python/mozbuild/mozbuild/artifacts.py:773
(Diff revision 3)
>  
> +        target_suffix = ''
> +
> +        # Add the "-debug" suffix to the guessed artifact job name
> +        # if MOZ_DEBUG is enabled.
> +        if buildconfig.substs.get('MOZ_DEBUG', '') == '1':

Just `if buildconfig.substs.get('MOZ_DEBUG'):`.  Empty strings are false-y in Python, and we don't care if we get exactly '1'.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8743834 [details]
MozReview Request: Bug 1253697 - Support downloading debug artifact builds. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48059/diff/3-4/

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/48059/#review45837

> nit: Revert these changes; they're not needed.

The above change has been reverted in the last push to mozreview (and the related logged messages in the _guess_artifact_job removed accordingly)

> Just `if buildconfig.substs.get('MOZ_DEBUG'):`.  Empty strings are false-y in Python, and we don't care if we get exactly '1'.

Agree, done in the last push to mozreview (and my apologies for my "rusty" python)

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Nick Alexander :nalexander from comment #10)
> Comment on attachment 8743834 [details]
> MozReview Request: Bug 1253697 - Download debug artifact builds on
> ac_add_options --debug-artifact. r?nalexander
> 
> https://reviewboard.mozilla.org/r/48059/#review45837
> 
> Looking better!  I have a few nits, and then we're good to go.

That's Great! 
I just pushed to mozreview the tweaks related to the nits described above.

Thanks a lot, Nick.

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8743834 [details]
MozReview Request: Bug 1253697 - Support downloading debug artifact builds. r=nalexander

https://reviewboard.mozilla.org/r/48059/#review46101

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8743834 [details]
MozReview Request: Bug 1253697 - Support downloading debug artifact builds. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48059/diff/4-5/

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

I've just pushed an update which tweaks the commit message 
(because the old commit message makes reference to the --debug-artifact option which was added in the first version of the patch and it doesn't exist anymore)

can checkin-needed be used to schedule this patch for landing?
(My apologies for the above question, but this is the first time that I'm landing a mozbuild patch, and without the possibility to push it to try first, and so I thought it worth a double-check before proceeding)

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to Luca Greco [:rpl] from comment #16)
> I've just pushed an update which tweaks the commit message 
> (because the old commit message makes reference to the --debug-artifact
> option which was added in the first version of the patch and it doesn't
> exist anymore)
> 
> can checkin-needed be used to schedule this patch for landing?
> (My apologies for the above question, but this is the first time that I'm
> landing a mozbuild patch, and without the possibility to push it to try
> first, and so I thought it worth a double-check before proceeding)

Yep, checkin-needed is good.  I'm setting it now.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36c2c8a36ad9

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36c2c8a36ad9