Closed
Bug 1357562
Opened 8 years ago
Closed 8 years ago
Do not replace any variant builds with artifact builds
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: chmanchester, Assigned: chmanchester)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
See dependent bugs. There may be other places this is happening, we need to make this do the right thing by default, probably by opting-in to artifact build replacement rather than out as Ted suggests in bug 1349180.
Assignee | ||
Comment 1•8 years ago
|
||
Hm, this is kind of tricky given how mozharness configs are structured as I understand it: the base configs contain what's necessary for a vanilla opt build, and that's added to or overridden by a sub config for any variany build (mozharness considers debug to be a variant build). So to specify this in the base config would be to specify it for the remaining variants, and we're back to opting out in variant configs.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → cmanchester
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8861172 [details]
Bug 1357562 - Specify artifact build replacement via mozharness for non-variant builds.
https://reviewboard.mozilla.org/r/133124/#review135994
::: testing/mozharness/scripts/fx_desktop_build.py:142
(Diff revision 1)
> #
> # This is temporary, until we find a way to introduce an "artifact
> # build dimension" like "opt"/"debug" into the CI configurations.
> self.info('Artifact build requested in try syntax.')
>
> - default = 'artifact'
> + original_variant = c.get('build_variant')
This is really
```
DEFAULT_VARIANT = 'debug-artifact' if c.get('build_variant') in DEBUG_VARIANTS else 'artifact'
c.get('artifact_flag_build_variant_in_try', DEFAULT_VARIANT)
```
right? I feel like this could be simplified, but I leave it to your judgement. (I think I worked through the cases to verify your refactor, but it's late and I'm tired.)
Attachment #8861172 -
Flags: review?(nalexander) → review+
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8861173 [details]
Bug 1357562 - Remove 'artifact_build_variant_in_try': None entries where no longer necessary.
https://reviewboard.mozilla.org/r/133126/#review135996
::: commit-message-0c9e6:3
(Diff revision 1)
> +Bug 1357562 - Remove 'artifact_build_variant_in_try': None entries where no longer necessary. r=nalexander
> +
> +A previous commit changed our default behavior to make these entries no longer
nit: _the_ previous commit, or just fold this into the previous commit.
Attachment #8861173 -
Flags: review?(nalexander) → review+
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8861174 [details]
Bug 1357562 - Prevent windows clang builds from being replaced by an artifact build.
https://reviewboard.mozilla.org/r/133128/#review135998
::: commit-message-98500:3
(Diff revision 1)
> +Bug 1357562 - Prevent windows clang builds from being replaced by an artifact build. r=nalexander
> +
> +These builds do not have a distinct variant but they do not have an artifact
nit: s/but/and/, or expand: "These builds do not have a distinct variant, so they default to 'artifact' when --artifact is passed to `mach try`, but they don't have an artifact build equivalent so ...
Attachment #8861174 -
Flags: review?(nalexander) → review+
Assignee | ||
Comment 8•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8861172 [details]
Bug 1357562 - Specify artifact build replacement via mozharness for non-variant builds.
https://reviewboard.mozilla.org/r/133124/#review135994
> This is really
> ```
> DEFAULT_VARIANT = 'debug-artifact' if c.get('build_variant') in DEBUG_VARIANTS else 'artifact'
> c.get('artifact_flag_build_variant_in_try', DEFAULT_VARIANT)
> ```
> right? I feel like this could be simplified, but I leave it to your judgement. (I think I worked through the cases to verify your refactor, but it's late and I'm tired.)
Not exactly... the essence of this change is that if we have a variant and it's not in DEBUG_VARIANTS we don't provide a replacement by default. I'll see if this can be simplified to be more obvious.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8861172 -
Flags: review+ → review?(nalexander)
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8861172 [details]
Bug 1357562 - Specify artifact build replacement via mozharness for non-variant builds.
https://reviewboard.mozilla.org/r/133124/#review137010
If you're happy, I am too!
Attachment #8861172 -
Flags: review?(nalexander) → review+
Comment 13•8 years ago
|
||
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/50295481fb98
Specify artifact build replacement via mozharness for non-variant builds. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/4b9adc99502a
Remove 'artifact_build_variant_in_try': None entries where no longer necessary. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/2d6216823293
Prevent windows clang builds from being replaced by an artifact build. r=nalexander
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/50295481fb98
https://hg.mozilla.org/mozilla-central/rev/4b9adc99502a
https://hg.mozilla.org/mozilla-central/rev/2d6216823293
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•