Closed Bug 1324998 Opened 7 years ago Closed 7 years ago

Including in-tree mozconfigs should be an error

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: glandium, Assigned: glandium)

Details

Attachments

(2 files)

Bug 1324998 - Set MOZ_AUTOMATION on rooting hazards builds.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1324998 - Error out when a in-tree mozconfig is included.
59 bytes, text/x-review-board-request
gps
: review+
Details 
Many people are shooting themselves in the foot by including in-tree mozconfigs, that have been meant to be used by automation for years now (but stale documentation still give bad examples).

We should just make it a hard error to include those files.
Attachment #8820593 - Flags: review?(gps)
Steve, is there a particular reason why the rooting hazard analysis builds don't set MOZ_AUTOMATION?
Flags: needinfo?(sphink)
A suggestion: rename or move the files so that it's clear that they are automation-specific?  build/mozconfig.common doesn't sound at all dangerous.
mak had a broken build just now because his mozconfig had `. $topsrcdir/browser/config/mozconfigs/common` which transitively included mozconfig.rust. That's a leftover incantation from a long time ago. Would it be caught by this?
Comment on attachment 8820593 [details]
Bug 1324998 - Set MOZ_AUTOMATION on rooting hazards builds.

https://reviewboard.mozilla.org/r/100094/#review100680

::: build/moz.configure/init.configure:240
(Diff revision 1)
> -def mozconfig_options(mozconfig, help):
> +def mozconfig_options(mozconfig, automation, help):
>      if mozconfig['path']:
> +        if 'MOZ_AUTOMATION_MOZCONFIG' in mozconfig['env']['added']:
> +            if not automation:
> +                log.error('%s directly or indirectly includes in-tree '
> +                          'mozconfigs.', mozconfig['path'])

I think saying why would also be helpful here. E.g.

```
log.error('%s directly or indirectly includes in-tree '
          'mozconfigs. These set options specific to '
          'the automation and official build environments '
          'and are not intended to work outside them.',
          mozconfig['path'])
```

Except with line wrapping.
(In reply to Mike Hommey [:glandium] from comment #2)
> Steve, is there a particular reason why the rooting hazard analysis builds
> don't set MOZ_AUTOMATION?

No, no reason. I didn't know it was useful/necessary. Feel free to add it. I notice that there are various MOZ_AUTOMATION_foo settings; if there are options for anything that consumes time or space, they should all be off. (The analysis only wants the compile. It doesn't care about the link or anything else that happens after the compile. But anything that results in more code being compiled is good.)
Flags: needinfo?(sphink)
Comment on attachment 8820593 [details]
Bug 1324998 - Set MOZ_AUTOMATION on rooting hazards builds.

https://reviewboard.mozilla.org/r/100094/#review100740

I agree with rillian that the error message should be approved. Please have the error point out the explicit paths of in-tree mozconfigs that are not allowed.
Attachment #8820593 - Flags: review?(gps) → review-
(In reply to Gregory Szorc [:gps] from comment #7)
> Comment on attachment 8820593 [details]
> Bug 1324998 - Error out when a in-tree mozconfig is included
> 
> https://reviewboard.mozilla.org/r/100094/#review100740
> 
> I agree with rillian that the error message should be approved. Please have
> the error point out the explicit paths of in-tree mozconfigs that are not
> allowed.

How do you suggest we find out the explicit paths of in-tree mozconfigs that were used? (because listing all of them is not going to help, there are well over a hundred)
(In reply to Mike Hommey [:glandium] from comment #8)
> How do you suggest we find out the explicit paths of in-tree mozconfigs that
> were used? (because listing all of them is not going to help, there are well
> over a hundred)

I suggest a generic message like "don't use the mozconfigs in the build/ directory."
(In reply to Gregory Szorc [:gps] from comment #9)
> (In reply to Mike Hommey [:glandium] from comment #8)
> > How do you suggest we find out the explicit paths of in-tree mozconfigs that
> > were used? (because listing all of them is not going to help, there are well
> > over a hundred)
> 
> I suggest a generic message like "don't use the mozconfigs in the build/
> directory."

Please would still feel like including mozconfigs from browser/config/mozconfigs is right, which it isn't. Really, there is no in-tree mozconfig that can be included, and this change makes pretty much sure of that.
Comment on attachment 8820593 [details]
Bug 1324998 - Set MOZ_AUTOMATION on rooting hazards builds.

https://reviewboard.mozilla.org/r/100094/#review101064

::: taskcluster/ci/hazard/kind.yml:22
(Diff revision 2)
>      worker:
>          implementation: docker-worker
>          max-run-time: 36000
>          docker-image: {in-tree: desktop-build}
> +        env:
> +            MOZ_AUTOMATION: "1"

How come this isn't in the mozharness configs like for other builds? Do users running hazard builds locally use mozharness directly?
Attachment #8820593 - Flags: review?(mshal) → review+
(In reply to Michael Shal [:mshal] from comment #13)
> > +        env:
> > +            MOZ_AUTOMATION: "1"
> 
> How come this isn't in the mozharness configs like for other builds? Do
> users running hazard builds locally use mozharness directly?

(1) Almost nobody runs hazard builds locally, to the extent that I have to fix them every time someone tries; and (2) hazard build don't use mozharness anymore (they haven't since switching to taskcluster). Last I used it, mozharness was just getting in the way; life got much much easier when I switched to shell scripts.

Which have since grown, as they tend to do, and I could certainly imagine switching to python at some point. And I wouldn't mind if those used some of our in-tree python libraries. But it would be a hard sell to get me back on mozharness; the effort to reward ratio last time was awful.

I will probably have to fix local builds again after the change in this bug to stop including automation mozconfigs. And I haven't even landed all of my last round of fixes.
(In reply to Michael Shal [:mshal] from comment #13)
> Comment on attachment 8820593 [details]
> Bug 1324998 - Set MOZ_AUTOMATION on rooting hazards builds.
> 
> https://reviewboard.mozilla.org/r/100094/#review101064
> 
> ::: taskcluster/ci/hazard/kind.yml:22
> (Diff revision 2)
> >      worker:
> >          implementation: docker-worker
> >          max-run-time: 36000
> >          docker-image: {in-tree: desktop-build}
> > +        env:
> > +            MOZ_AUTOMATION: "1"
> 
> How come this isn't in the mozharness configs like for other builds? Do
> users running hazard builds locally use mozharness directly?

Note, if it weren't for buildbot, I would push for MOZ_AUTOMATION to be set at the taskcluster level already.
Comment on attachment 8821080 [details]
Bug 1324998 - Error out when a in-tree mozconfig is included.

https://reviewboard.mozilla.org/r/100458/#review101716

I pushed to Try again because the last push was seeing failures. This one seems green, so r+.
Attachment #8821080 - Flags: review?(gps) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/1775ed1787ba
Set MOZ_AUTOMATION on rooting hazards builds. r=mshal
https://hg.mozilla.org/integration/autoland/rev/aa940ccb1d9f
Error out when a in-tree mozconfig is included. r=gps
https://hg.mozilla.org/mozilla-central/rev/1775ed1787ba
https://hg.mozilla.org/mozilla-central/rev/aa940ccb1d9f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
https://hg.mozilla.org/comm-central/rev/f2daf1f27a53fd0ee70adc5ee0c61d7a11542be0
Keep build/ in sync (Bug 1324998 - Error out when a in-tree mozconfig is included). rs=bustage-fix
Pushed by Callek@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/e120594f18fb
Followup for Bug 1324998, set MOZ_AUTOMATION for l10n. r=dustin rs-a=Kwierso
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: