[Merge] lp:~chunsang/media-hub/wip_media-hub-interface into lp:media-hub

Jim Hodapp jim.hodapp at canonical.com
Tue Mar 21 17:55:16 UTC 2017


Review: Needs Fixing code

A few things to fix. See inline below.

Diff comments:

> === added directory 'overlay'
> === added directory 'overlay/bin'
> === added file 'overlay/bin/media-hub-service.wrapper'
> --- overlay/bin/media-hub-service.wrapper	1970-01-01 00:00:00 +0000
> +++ overlay/bin/media-hub-service.wrapper	2017-03-19 07:35:27 +0000
> @@ -0,0 +1,26 @@
> +#!/bin/sh
> +#
> +# Copyright (C) 2017 Canonical Ltd
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License version 3 as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +if [ -e /var/run/pulse ]; then
> +  # PA as a snap
> +  export PULSE_RUNTIME_PATH=/var/run/pulse
> +  export PULSE_SYSTEM=1
> +else
> +  # classic
> +  export PULSE_RUNTIME_PATH=/run/user/$(id -u)/pulse
> +fi
> +
> +exec $SNAP/bin/desktop-launch "$@"

Is this really the right thing to launch media-hub with? Is this giving you something in the environment that simply doing: 'exec "media-hub-server" "$@"' can't accomplish?

> 
> === added directory 'overlay/ubuntu-app-platform'
> === modified file 'snapcraft.yaml'
> --- snapcraft.yaml	2017-02-17 15:56:44 +0000
> +++ snapcraft.yaml	2017-03-19 07:35:27 +0000
> @@ -75,6 +105,12 @@
>          - -usr/lib/*.la
>          - -usr/lib/*/*.la
>          - -usr/lib/*/*.o
> +    after:
> +      - desktop-ubuntu-app-platform
>  
> -    snap:
> +    prime:
>        - $unwanted
> +
> +  media-hub-overlay:

Can you add a comment as to what this section's purpose is just so it's clear to the next person who looks at this snapcraft.yaml file?

> +    source: overlay/
> +    plugin: dump
> 
> === modified file 'src/core/media/apparmor/ubuntu.cpp'
> --- src/core/media/apparmor/ubuntu.cpp	2016-04-06 15:42:15 +0000
> +++ src/core/media/apparmor/ubuntu.cpp	2017-03-19 07:35:27 +0000
> @@ -85,7 +89,7 @@
>      static const std::regex full_re{"(.*)_(.*)_(.*)"};
>      static const std::regex trust_store_re{"(.*)-(.*)"};
>  
> -    if ((s == "messaging-app" or s == unity_name)
> +    if ((s == "messaging-app" or s == unity_name or s == unity8_snap_name or s == mediaplayer_snap_name)

You may want to check with ahayzen quickly and add the snap name for music-app as well while we're modifying this section. Should be a very easy change.

>              and std::regex_match(s, out, trust_store_re))
>      {
>          pkg_name = s;


-- 
https://code.launchpad.net/~chunsang/media-hub/wip_media-hub-interface/+merge/319896
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.



More information about the Ubuntu-reviews mailing list