1227182 - Provide OpenH264 v1.5.3 build

Status: RESOLVED FIXED

(Release Engineering :: Release Requests, defect)

Description

[Justin Wood (:Callek)]

+++ This bug was initially created as a clone of Bug #1223925 +++

+++ This bug was initially created as a clone of Bug #1217475 +++

Comment 1

[Justin Wood (:Callek)]

This is now done, submitted to Cisco, and pre-emptively pointed nightlytest at the resting place of the binaries once cisco puts them up.

Comment 2

[Justin Wood (:Callek)]

(In reply to Justin Wood (:Callek) from comment #1)
> This is now done, submitted to Cisco, and pre-emptively pointed nightlytest
> at the resting place of the binaries once cisco puts them up.

Turns out the first go didn't yield a fix for our android issue, I'm doing a new build, ETA for handoff is by EOD ET tomorrow.

This next push is going to involve also doing symbol files for our Socorro server and some improvements in our build process (making the efforts easy)

Comment 3

[Justin Wood (:Callek)]

Attachment: [inbound] mozharness changes

r? to catlee, primarily for the non openh264 changes, (the openh264 I'd be ok landing without review, since NPOTDB but review is still useful)

I'm using these changes to do this round of the build. I also plan to eventually write code for auto-upload of symbols and such, but this is enough of a win for now.

Comment 4

[Jordan Lund (:jlund)]

Comment on attachment 8698758 [details] [diff] [review]
[inbound] mozharness changes

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

overall, this beast looks great! :D No real issues for me but I would like to point out I have little context into the requirements or expected logic. I mainly looked at the code itself. Would be get to have a second pair of eyes to glance over the bigger picture.

I didn't look at packagesymbols.py. Is that a copy from something else? Is that why you put it in external_tools instead of making it part of mozharness?

::: testing/mozharness/mozharness/base/script.py
@@ +1279,3 @@
>              shell = False
> +
> +        if env is None:

how about instead putting the onus on the caller of get_output_from_command:

either:
1) get_output_from_command(env=self.query_env())
2) get_output_from_command(env=self.query_env(partial_env={foo:bar})

::: testing/mozharness/scripts/openh264_build.py
@@ +142,1 @@
>      def query_package_name(self):

can this whole thing be configurable?

@@ +150,5 @@
>          if sys.platform == 'linux2':
>              if self.config.get('operating_system') == 'android':
> +                if self.config.get('arch') == 'x86':
> +                    return 'openh264-android-{arch}-{version}.zip'.format(
> +                        version=version, bits=bits, arch=self.config['arch']

no bits in this string

@@ +154,5 @@
> +                        version=version, bits=bits, arch=self.config['arch']
> +                    )
> +                else:
> +                    return 'openh264-android-{arch}-{version}.zip'.format(
> +                        version=version, bits=bits, arch='arm'

same here, no bits in this string

@@ +178,5 @@
>  
>          if "operating_system" in self.config:
>              retval.append("OS=%s" % self.config['operating_system'])
> +            if self.config["operating_system"] == "android":
> +                if self.config['arch'] == 'x86':

again here, can this be configurable in the android config so we have less script conditions? do we need --os ?

e.g. (in openh264/android-x86.py config):

config = {

'x86':
    'make_params':
         'TARGET': 'invalid',
         # ... etc

}

@@ +206,4 @@
>          cmd = ['make', target] + self.query_make_params()
>          dirs = self.query_abs_dirs()
>          repo_dir = os.path.join(dirs['abs_work_dir'], 'src')
> +        env = self.config.get('env')

is this ever going to have a value? do you want the os.environment ever with config['env']? e.g. self.query_env()

@@ +206,5 @@
>          cmd = ['make', target] + self.query_make_params()
>          dirs = self.query_abs_dirs()
>          repo_dir = os.path.join(dirs['abs_work_dir'], 'src')
> +        env = self.config.get('env')
> +        partial_env = self.config.get('partial_env')

passing partial_env and env opens us for risk of both one day having a value.

iiuc, the following would be equivalent:

env = self.query_env(
    partial_env=self.config.get('partial_env'),  # windows
    avoid_host_env=True
)

@@ +267,5 @@
>              os.unlink(package_file)
> +        to_package = []
> +        for f in glob.glob(os.path.join(srcdir, "*gmpopenh264*")):
> +            if not re.search(
> +                    "(?:lib)?gmpopenh264(?!\.\d)\.(?:dylib|so|dll|info)(?!\.\d)",

I'm guessing there is no way we can make this or the *gmpopenh264* an explicit manifest?

@@ +285,5 @@
> +    def dump_symbols(self):
> +        dirs = self.query_abs_dirs()
> +        c = self.config
> +        srcdir = os.path.join(dirs['abs_work_dir'], 'src')
> +        package_name = self.run_make('echo-plugin-name', capture_output=True)

var names are a bit hard to grasp from noob outside eyes.

what's the difference between 'echo-plugin-name' and self.query_package_name? they both seem to be _a_ package name. the latter is obviously zipped. but are they different packages? Should this be renamed to plugin_name?

@@ +293,5 @@
> +        if not zip_package_name[-4:] == ".zip":
> +            self.fatal("Unexpected zip_package_name")
> +        symbol_package_name = "{base}.symbols.zip".format(base=zip_package_name[:-4])
> +        symbol_zip_path = os.path.join(dirs['abs_upload_dir'], symbol_package_name)
> +        repo_dir = os.path.join(dirs['abs_work_dir'], 'src')

iiuc, this is the same as the above defined srcdir

@@ +295,5 @@
> +        symbol_package_name = "{base}.symbols.zip".format(base=zip_package_name[:-4])
> +        symbol_zip_path = os.path.join(dirs['abs_upload_dir'], symbol_package_name)
> +        repo_dir = os.path.join(dirs['abs_work_dir'], 'src')
> +        env = self.config.get('env')
> +        partial_env = self.config.get('partial_env')

same things for env/partial_env here

Comment 5

[Justin Wood (:Callek)]

1.5.2 was a wash, moving bug to reflect 1.5.3

Comment 6

[Justin Wood (:Callek)]

(In reply to Jordan Lund (:jlund) from comment #4)
> Comment on attachment 8698758 [details] [diff] [review]
> [inbound] mozharness changes
> 
> Review of attachment 8698758 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> overall, this beast looks great! :D No real issues for me but I would like
> to point out I have little context into the requirements or expected logic.
> I mainly looked at the code itself. Would be get to have a second pair of
> eyes to glance over the bigger picture.
> 

I'll flesh these out in another bug, since the time has come that I need to actually build... None of your comments block my build of openh264 itself, so I'm proceeding...

Comment 7

[Justin Wood (:Callek)]

This build I'm using rev 2706e36bf0a8b7c539c803ed877148c005ffca59  from the v1.5.3-Firefox39 branch.

TRY_REV below is the rev for the try push of the above patch...

OSX:

export OPENH264_REV=...
export TRY_REV=...
cd /builds/slave
rm -rf openh264*
svn co https://googletest.googlecode.com/svn/trunk/ gtest
#  Accept permanently
rm -rf gtest
for config in macosx32 macosx64; do mkdir openh264_$config;
pushd openh264_$config;
hg clone https://hg.mozilla.org/build/tools;
bash -c 'python tools/buildfarm/utils/archiver_client.py mozharness --repo try --rev $TRY_REV --destination scripts --debug';
python scripts/scripts/openh264_build.py --config scripts/configs/openh264/$config.py --rev $OPENH264_REV;
python scripts/scripts/openh264_build.py --config scripts/configs/openh264/$config.py --rev $OPENH264_REV --upload;
popd;
done


Linux/Android:

export OPENH264_REV=...
export TRY_REV=...
cd /builds/slave
rm -rf openh264*
for config in linux32 linux64 android-arm android-x86; do mkdir openh264_$config;
pushd openh264_$config;
hg clone https://hg.mozilla.org/build/tools;
bash -c 'python tools/buildfarm/utils/archiver_client.py mozharness --repo try --rev $TRY_REV --destination scripts --debug';
python scripts/scripts/openh264_build.py --config scripts/configs/openh264/$config.py --rev $OPENH264_REV;
python scripts/scripts/openh264_build.py --config scripts/configs/openh264/$config.py --rev $OPENH264_REV --upload;
popd;
done

=========

After that I transferred the files to partner-repacks to sign windows

Then I uploaded the symbols to the symbol server using the in-tree upload_symbols.py for me.

And pushed the binary files to cisco.
Windows:

export OPENH264_REV=...
export TRY_REV=...
cd /c/builds/moz2_slave
rm -rf openh264*
for config in win32 win64; do mkdir openh264_$config;
pushd openh264_$config;
hg clone https://hg.mozilla.org/build/tools;
bash -c 'python tools/buildfarm/utils/archiver_client.py mozharness --repo try --rev $TRY_REV --destination scripts --debug';
python scripts/scripts/openh264_build.py --config scripts/configs/openh264/$config.py --rev $OPENH264_REV;
python scripts/scripts/openh264_build.py --config scripts/configs/openh264/$config.py --rev $OPENH264_REV --upload;
popd;
done

Comment 8

[Justin Wood (:Callek)]

This was signed for windows, and then symbols pushed to our symbol store and handed off to cisco.

I'll leave the review request on cat.lee open and follow up on automation improvements as time allows, until the next release ;-)

Comment 9

[Chris AtLee [:catlee]]

Comment on attachment 8698758 [details] [diff] [review]
[inbound] mozharness changes

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

overall looks fine - the code changes to core mozharness modules scares me a bit though. is there another way to do that?

::: testing/mozharness/mozharness/base/script.py
@@ +1279,3 @@
>              shell = False
> +
> +        if env is None:

yeah, I agree. I don't like changing this base method just for this.

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 8698758 [details] [diff] [review]
[inbound] mozharness changes

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

::: testing/mozharness/mozharness/base/script.py
@@ +1279,3 @@
>              shell = False
> +
> +        if env is None:

admittedly, this is the same syntax-style for partial_env as we do in run_command. I was trying to match it.

e.g. https://dxr.mozilla.org/mozilla-central/rev/c5da92c5b4906369dee83629f81d647226ac1038/testing/mozharness/mozharness/base/script.py#1133