Closed Bug 1253697 Opened 4 years ago Closed 3 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: whimboo, Assigned: rpl, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=python][good next bug])

Attachments

(1 file)

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
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.
Follow-on to Bug 1243041.  Might be a good mentor bug.
Mentor: cmanchester, nalexander
Depends on: 1243041
Whiteboard: [lang=python][good next bug]
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!
(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 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/
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 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/
Attachment #8743834 - Attachment description: MozReview Request: Bug 1253697 - Download debug artifact builds on ac_add_options --debug-artifact. → MozReview Request: Bug 1253697 - Download debug artifact builds on ac_add_options --debug-artifact. r?nalexander
Attachment #8743834 - Flags: review?(nalexander)
(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 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'.
Attachment #8743834 - Flags: review?(nalexander)
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/
Attachment #8743834 - Flags: review?(nalexander)
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)
(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.
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Comment on attachment 8743834 [details]
MozReview Request: Bug 1253697 - Support downloading debug artifact builds. r=nalexander

https://reviewboard.mozilla.org/r/48059/#review46101
Attachment #8743834 - Flags: review?(nalexander) → review+
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/
Attachment #8743834 - Attachment description: MozReview Request: Bug 1253697 - Download debug artifact builds on ac_add_options --debug-artifact. r?nalexander → MozReview Request: Bug 1253697 - Support downloading debug artifact builds. r=nalexander
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)
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/36c2c8a36ad9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1276037
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.