Closed Bug 1178286 Opened 5 years ago Closed 4 years ago

switch release automation source builder to taskcluster

Categories

(Release Engineering :: Release Automation: Other, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
Tracking Status
firefox47 --- fixed

People

(Reporter: bhearsum, Assigned: rail)

References

Details

Attachments

(5 files, 2 obsolete files)

This includes task definition and any necessary changes to the script that does this job. If the we have an initial task graph by the time this gets looked at, it should integrate with that, too.

Rail, you said you wanted to use this builder as a test case for trialing out a few things. Assign to you
Duplicate of this bug: 748796
Attached patch [WIP] source-mozharness.diff (obsolete) — Splinter Review
Switching to mozharness at the same time. Looks like we already have support to package sources there, but it needs some tweaks.
Depends on: 1179289
Attached patch source-railz-mozharness-3.diff (obsolete) — Splinter Review
This patch:
 * updates the existing config with missing variables
 * refactors the package_source to use a new wrapper called check_run_command_m() - not sure if it belongs to BuildScript, though.
 * uses `mach configure' instead of `make -f client.mk configure'
 * no need to upload, TC handles that

Example job: https://tools.taskcluster.net/task-inspector/#fn98oVkaQcafZSh89OTVuw/0

A diff between `find | sort > list.txt` of existing 39.0 tarball (on FTP) and the generated by the task above: https://gist.github.com/rail/fff90a1920c2bf444f21
Attachment #8627805 - Attachment is obsolete: true
Attachment #8628458 - Flags: review?(jlund)
Attached file task.json
FTR, this is the task definition I used
Comment on attachment 8628458 [details] [diff] [review]
source-railz-mozharness-3.diff

Review of attachment 8628458 [details] [diff] [review]:
-----------------------------------------------------------------

looks great! I can't attest to the targets you are calling and needed cmd args but the code looks good to me. Small questions/comments below.

::: configs/builds/releng_sub_linux_configs/64_source.py
@@ +10,5 @@
>      'purge_minsize': 3,
> +    'buildbot_json_path': 'buildprops.json',
> +    'app_ini_path': 'FAKE',  # Not used, but required by the script
> +    'objdir': 'obj-firefox',
> +    'env': {

sanity check as your env looks minimal: this won't append to any mh base config that you are using in the script call, it will overwrite. Ignore me if you only want 3 vars + os.environ

::: mozharness/mozilla/building/buildbase.py
@@ +1695,5 @@
>                                    halt_on_failure=True)
>          if self.config.get('enable_ccache'):
>              self._ccache_s()
>  
> +    def check_run_command_m(self, *args, **kwargs):

neat! I wonder if this wrapper could go into mock: http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/mock.py

@@ +1712,5 @@
> +
> +    def _create_empty_mozconfig(self):
> +        """Create an empty mozconfig file"""
> +        self.log("creating an empty mozconfig...")
> +        with open(os.path.join(self.query_abs_dirs()['abs_src_dir'],

mh has a _touch_file[1] that comes with some logging and exception catching for free. might be applicable here.

@@ +1730,5 @@
>          )
> +        self.check_run_command_m(
> +            command=[
> +                'make', 'source-package', 'hg-bundle',
> +                'HG_BUNDLE_REVISION=%s' % self.query_revision(),

does the env var have to go into the command? I know some make targets required that on certain platforms but if not, extending `env` seems more natural.
Attachment #8628458 - Flags: review?(jlund) → review+
interdiff: https://gist.github.com/rail/b7197e5d4c584fd8ade1

Thanks for the comments, they made me simplify the patch.

(In reply to Jordan Lund (:jlund) from comment #6)
> sanity check as your env looks minimal: this won't append to any mh base
> config that you are using in the script call, it will overwrite. Ignore me
> if you only want 3 vars + os.environ

I tried to keep the env as much as possible less polluted. It perfectly works with those 3 variables set.
 
> ::: mozharness/mozilla/building/buildbase.py
> @@ +1695,5 @@
> >                                    halt_on_failure=True)
> >          if self.config.get('enable_ccache'):
> >              self._ccache_s()
> >  
> > +    def check_run_command_m(self, *args, **kwargs):
> 
> neat! I wonder if this wrapper could go into mock:
> http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/mock.py

Bah, it turns out that I reimplemented halt_on_failure! :) Dropping this from the patch. I don't need any fancy status logic here. If it fails, it's red.


> mh has a _touch_file[1] that comes with some logging and exception catching
> for free. might be applicable here.

Bah #2! I used self._touch_file() instead.
 
> @@ +1730,5 @@
> >          )
> > +        self.check_run_command_m(
> > +            command=[
> > +                'make', 'source-package', 'hg-bundle',
> > +                'HG_BUNDLE_REVISION=%s' % self.query_revision(),
> 
> does the env var have to go into the command? I know some make targets
> required that on certain platforms but if not, extending `env` seems more
> natural.

Even though it looks like an env variable, it's not. It's passed to make as an argument and make uses it to (re)define internal variables.
Attachment #8628458 - Attachment is obsolete: true
Attachment #8628760 - Flags: review?(jlund)
Comment on attachment 8628760 [details] [diff] [review]
source-railz-mozharness-4.diff

Review of attachment 8628760 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm! :D
Attachment #8628760 - Flags: review?(jlund) → review+
Depends on: 1197207
Blocks: 1149803
Can't use system wide GCC anymore :/
Have to switch to tooltool
Attachment #8677192 - Flags: review?(jlund)
Still need to fix the artifacts (they are gone now)
Comment on attachment 8677192 [details] [diff] [review]
source_update.diff

Review of attachment 8677192 [details] [diff] [review]:
-----------------------------------------------------------------

shweet
Attachment #8677192 - Flags: review?(jlund) → review+
Keywords: leave-open
It turns out that the current script doesn't handle DIST_TARGET_UPLOADS in https://github.com/mozilla/releasetasks/blob/master/releasetasks/templates/source.yml.tmpl#L51

We need to call "make upload", which in TC case will set UPLOAD_HOST to "localhost" in https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/builds/build_pool_specifics.py#46

In this case upload.py just copies files to UPLOAD_PATH, see https://dxr.mozilla.org/mozilla-central/source/build/upload.py#18

To make the solution solid, we could add a new mach command to handle source generation. I have a patch at https://gist.github.com/rail/401b7b9681fb6f44f5e1 which needs some love:

* It times out. The commands take more than default 180 secs. Probably need to tweak the timeout or make the commands print something (not sure if it works in hg-bundle case)
* No default value for --revision
* Maybe make hg-bundle optional
I pushed https://hg.mozilla.org/projects/date/rev/c6f40a198297 to date to disable tooltool all together, because the new mozconfig doesn't require it.
and https://hg.mozilla.org/projects/date/rev/c6c535a4c21e to fix a typo (missing coma).
* no more tooltool with "source" mozconfig
* fix targets to upload files for reals
* generate signing manifest
Attachment #8722112 - Flags: review?(jlund)
Attached file fix source tasks
Attachment #8722113 - Flags: review?(jlund)
Attachment #8722112 - Flags: review?(jlund) → review+
Attachment #8722113 - Flags: review?(jlund) → review+
Comment on attachment 8722113 [details] [review]
fix source tasks

merged
Attachment #8722113 - Flags: checked-in+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/94d790995f2c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.