[Merge] lp:~sergiusens/goget-ubuntu-touch/readOEMPackageYaml into lp:goget-ubuntu-touch
Michael Vogt
michael.vogt at canonical.com
Fri Mar 27 14:46:10 UTC 2015
Review: Approve
Thanks, looks good, I have some inline comments, but nothing relevant.
Diff comments:
> === modified file 'diskimage/common.go'
> --- diskimage/common.go 2015-01-28 19:37:00 +0000
> +++ diskimage/common.go 2015-03-27 13:26:46 +0000
> @@ -53,7 +53,7 @@
> type CoreImage interface {
> Image
> SystemImage
> - SetupBoot(OemDescription) error
> + SetupBoot() error
> FlashExtra(string) error
> }
>
> @@ -66,17 +66,46 @@
> }
>
> type OemDescription struct {
> - Name string `yaml:"name"`
> - Version string `yaml:"version"`
> - Hardware struct {
> - Dtb string `yaml:"dtb,omitempty"`
> - } `yaml:"hardware,omitempty"`
> + Name string `yaml:"name"`
> + Version string `yaml:"version"`
> +
> + OEM *struct {
> + Hardware *struct {
> + Bootloader string `yaml:"bootloader"`
> + PartitionLayout string `yaml:"partition-layout"`
> + Dtb string `yaml:"dtb,omitempty"`
> + Platform string `yaml:"platform"`
> + Architecture string `yaml:"architecture"`
> + } `yaml:"hardware,omitempty"`
> +
> + Store *struct {
> + ID string `yaml:"id,omitempty"`
> + }
> + } `yaml:"oem,omitempty"`
> +
> + Config map[string]interface{} `yaml:"config,omitempty"`
> }
>
> func (o OemDescription) InstallPath() string {
> return filepath.Join("/oem", o.Name, o.Version)
> }
>
> +func (o OemDescription) Architecture() string {
> + return o.OEM.Hardware.Architecture
> +}
> +
> +func (o OemDescription) PartitionLayout() string {
> + return o.OEM.Hardware.PartitionLayout
> +}
> +
> +func (o OemDescription) Platform() string {
> + return o.OEM.Hardware.Platform
> +}
> +
> +func (o *OemDescription) SetPlatform(platform string) {
> + o.OEM.Hardware.Platform = platform
> +}
> +
> func sectorSize(dev string) (string, error) {
> out, err := exec.Command("blockdev", "--getss", dev).CombinedOutput()
> if err != nil {
>
> === modified file 'diskimage/core_grub.go'
> --- diskimage/core_grub.go 2015-01-29 14:26:27 +0000
> +++ diskimage/core_grub.go 2015-03-27 13:26:46 +0000
> @@ -281,7 +281,7 @@
> return img.baseMount
> }
>
> -func (img *CoreGrubImage) SetupBoot(oem OemDescription) error {
> +func (img *CoreGrubImage) SetupBoot() error {
> for _, dev := range []string{"dev", "proc", "sys"} {
> src := filepath.Join("/", dev)
> dst := filepath.Join(img.System(), dev)
>
> === modified file 'diskimage/core_uboot.go'
> --- diskimage/core_uboot.go 2015-03-04 14:47:07 +0000
> +++ diskimage/core_uboot.go 2015-03-27 13:26:46 +0000
> @@ -39,11 +39,11 @@
> CoreImage
> SystemImage
> hardware HardwareDescription
> + oem OemDescription
> location string
> size int64
> baseMount string
> parts []partition
> - platform string
> }
>
> const snappySystemTemplate = `# This is a snappy variables and boot logic file and is entirely generated and
> @@ -79,12 +79,12 @@
> Bootloader []string `yaml:"bootloader"`
> }
>
> -func NewCoreUBootImage(location string, size int64, hw HardwareDescription, platform string) *CoreUBootImage {
> +func NewCoreUBootImage(location string, size int64, hw HardwareDescription, oem OemDescription) *CoreUBootImage {
> return &CoreUBootImage{
> hardware: hw,
> + oem: oem,
> location: location,
> size: size,
> - platform: platform,
> }
> }
>
> @@ -290,7 +290,7 @@
> return img.baseMount
> }
>
> -func (img CoreUBootImage) SetupBoot(oem OemDescription) error {
> +func (img CoreUBootImage) SetupBoot() error {
> // destinations
> bootPath := filepath.Join(img.baseMount, string(bootDir))
> bootAPath := filepath.Join(bootPath, "a")
> @@ -331,7 +331,7 @@
> return err
> }
>
> - if err := img.provisionDtbs(oem, bootDtbPath); err != nil {
> + if err := img.provisionDtbs(bootDtbPath); err != nil {
> return err
> }
> }
> @@ -352,8 +352,8 @@
> defer snappySystemFile.Close()
>
> var fdtfile string
> - if img.platform != "" {
> - fdtfile = fmt.Sprintf("fdtfile=%s.dtb", img.platform)
> + if platform := img.oem.Platform(); platform != "" {
> + fdtfile = fmt.Sprintf("fdtfile=%s.dtb", platform)
> }
>
> t := template.Must(template.New("snappy-system").Parse(snappySystemTemplate))
> @@ -363,12 +363,14 @@
> }
>
> func (img CoreUBootImage) provisionUenv(bootuEnvPath string) error {
> - if img.platform == "" {
> + platform := img.oem.Platform()
> +
> + if platform != "" {
> printOut("No platform select, not searching for uEnv.txt")
> return nil
> }
>
> - flashAssetsPath := filepath.Join(img.baseMount, "flashtool-assets", img.platform)
> + flashAssetsPath := filepath.Join(img.baseMount, "flashtool-assets", platform)
> uEnvPath := filepath.Join(flashAssetsPath, "uEnv.txt")
>
> if _, err := os.Stat(flashAssetsPath); os.IsNotExist(err) {
> @@ -391,7 +393,7 @@
> return nil
> }
>
> -func (img CoreUBootImage) provisionDtbs(oem OemDescription, bootDtbPath string) error {
> +func (img CoreUBootImage) provisionDtbs(bootDtbPath string) error {
> dtbsPath := filepath.Join(img.baseMount, img.hardware.Dtbs)
>
> if _, err := os.Stat(dtbsPath); os.IsNotExist(err) {
> @@ -405,12 +407,12 @@
> return err
> }
>
> - dtb := filepath.Join(dtbsPath, fmt.Sprintf("%s.dtb", img.platform))
> + dtb := filepath.Join(dtbsPath, fmt.Sprintf("%s.dtb", img.oem.Platform()))
>
> // if there is a specific dtb for the platform, copy it.
> // First look in oem and then in device.
> - if oem.Hardware.Dtb != "" && img.platform != "" {
> - oemDtb := filepath.Join(img.System(), oem.InstallPath(), oem.Hardware.Dtb)
> + if oemDtb := img.oem.OEM.Hardware.Dtb; oemDtb != "" && img.oem.Platform() != "" {
> + oemDtb := filepath.Join(img.System(), img.oem.InstallPath(), oemDtb)
> dst := filepath.Join(bootDtbPath, filepath.Base(dtb))
> if err := copyFile(oemDtb, dst); err != nil {
> return err
> @@ -434,7 +436,7 @@
> }
>
> func (img *CoreUBootImage) FlashExtra(devicePart string) error {
> - if img.platform == "" {
> + if img.oem.Platform() == "" {
> return nil
> }
>
> @@ -454,7 +456,7 @@
> return nil
> }
>
> - flashAssetsPath := filepath.Join(tmpdir, "flashtool-assets", img.platform)
> + flashAssetsPath := filepath.Join(tmpdir, "flashtool-assets", img.oem.Platform())
> flashPath := filepath.Join(flashAssetsPath, "flash.yaml")
>
> if _, err := os.Stat(flashPath); err != nil && os.IsNotExist(err) {
>
> === modified file 'ubuntu-device-flash/channel_maps.go'
> --- ubuntu-device-flash/channel_maps.go 2015-03-27 13:26:46 +0000
> +++ ubuntu-device-flash/channel_maps.go 2015-03-27 13:26:46 +0000
> @@ -41,3 +41,28 @@
>
> return channel
> }
> +
> +const (
This could be implemented as a map unless I'm mistaken:
const archDeviceMapping map[string]string{"armhf": "generic_armhf", "amd64": "generic_amd64", } which would be slightly compacter. Not really important though and I just noticed that the consts are used below for other purposes so…nevermind.
> + archArmhf = "armhf"
> + archAmd64 = "amd64"
> + archi386 = "i386"
> +)
> +
> +const (
> + deviceArmhf = "generic_armhf"
> + deviceAmd64 = "generic_amd64"
> + devicei386 = "generic_i386"
> +)
> +
> +func systemImageDeviceChannel(arch string) string {
> + switch arch {
> + case archArmhf:
> + return deviceArmhf
> + case archAmd64:
> + return deviceAmd64
> + case archi386:
> + return devicei386
> + }
> +
> + return arch
> +}
>
> === modified file 'ubuntu-device-flash/core.go'
> --- ubuntu-device-flash/core.go 2015-03-27 13:26:46 +0000
> +++ ubuntu-device-flash/core.go 2015-03-27 13:26:46 +0000
> @@ -57,6 +57,7 @@
> Cloud bool `long:"cloud" description:"Generate a pure cloud image without setting up cloud-init"`
> Platform string `long:"platform" description:"specify the boards platform"`
> Install []string `long:"install" description:"install additional packages (can be called multiple times)"`
> + Oem string `long:"oem" descripion:"the snappy oem package to base the image out of"`
>
> Development struct {
> DevicePart string `long:"device-part" description:"Specify a local device part to override the one from the server"`
> @@ -66,6 +67,7 @@
> } `group:"Development"`
>
> hardware diskimage.HardwareDescription
> + oem diskimage.OemDescription
> }
>
> var coreCmd CoreCmd
> @@ -95,6 +97,11 @@
> devicePart = p
> }
>
> + fmt.Println("Determining oem configuration")
> + if err := coreCmd.extractOemDescription(coreCmd.Oem); err != nil {
> + return err
> + }
> +
> if !globalArgs.DownloadOnly {
> if syscall.Getuid() != 0 {
> return errors.New("command requires sudo/pkexec (root)")
> @@ -109,14 +116,21 @@
> }
>
> fmt.Println("Fetching information from server...")
> -
> channels, err := ubuntuimage.NewChannels(globalArgs.Server)
> if err != nil {
> return err
> }
>
> channel := systemImageChannel(coreCmd.Channel)
> - deviceChannel, err := channels.GetDeviceChannel(globalArgs.Server, channel, coreCmd.Device)
> +
> + var device string
> + if coreCmd.oem.Architecture() != "" {
> + device = systemImageDeviceChannel(coreCmd.oem.Architecture())
> + } else {
> + device = systemImageDeviceChannel(coreCmd.Device)
> + }
> +
> + deviceChannel, err := channels.GetDeviceChannel(globalArgs.Server, channel, device)
> if err != nil {
> return err
> }
> @@ -202,11 +216,16 @@
> return nil
> }
>
> - switch coreCmd.hardware.Bootloader {
> + // for backwards compatibility
> + if coreCmd.oem.OEM.Hardware.Bootloader == "" {
> + coreCmd.oem.OEM.Hardware.Bootloader = coreCmd.hardware.Bootloader
> + }
> +
> + switch coreCmd.oem.OEM.Hardware.Bootloader {
> case "grub":
> img = diskimage.NewCoreGrubImage(coreCmd.Output, coreCmd.Size)
> case "u-boot":
> - img = diskimage.NewCoreUBootImage(coreCmd.Output, coreCmd.Size, coreCmd.hardware, coreCmd.Platform)
> + img = diskimage.NewCoreUBootImage(coreCmd.Output, coreCmd.Size, coreCmd.hardware, coreCmd.oem)
> default:
> fmt.Printf("Bootloader set to '%s' in hardware description, assuming grub as a fallback\n", coreCmd.hardware.Bootloader)
> img = diskimage.NewCoreGrubImage(coreCmd.Output, coreCmd.Size)
> @@ -261,7 +280,7 @@
> }
>
> fmt.Println("New image complete")
> - if coreCmd.Device != "generic_armhf" {
> + if coreCmd.oem.OEM.Hardware.Architecture != "armhf" {
> fmt.Println("Launch by running: kvm -m 768", coreCmd.Output)
> }
>
> @@ -325,13 +344,7 @@
> return err
> }
>
> - // check if we installed an oem snap
> - oem, err := loadOem(systemPath)
> - if err != nil {
> - return err
> - }
> -
> - if err := img.SetupBoot(oem); err != nil {
> + if err := img.SetupBoot(); err != nil {
> return err
> }
>
> @@ -353,7 +366,7 @@
>
> // if the device is armhf, we can't to make this copy here since it's faster
> // than on the device.
> - if coreCmd.Device == "generic_armhf" && coreCmd.hardware.PartitionLayout == "system-AB" {
> + if coreCmd.oem.Architecture() == archArmhf && coreCmd.oem.PartitionLayout() == "system-AB" {
> printOut("Replicating system-a into system-b")
>
> src := fmt.Sprintf("%s/.", systemPath)
> @@ -377,9 +390,25 @@
> flags |= snappy.AllowUnauthenticated
> }
>
> - for _, snap := range coreCmd.Install {
> - snapBase := filepath.Base(snap)
> - fmt.Println("Installing", snapBase)
> + packageCount := len(coreCmd.Install) + len(coreCmd.oem.Config)
> + if coreCmd.Oem != "" {
> + packageCount += 1
> + }
> + packageQueue := make([]string, 0, packageCount)
Just curious, does this really matter? Or could "packageQueue := []string{nil}" and you just grow it via the append()? Or is that a much less efficient operation (i.e. lots of reallocs)? Its fine, I'm just curious.
> +
> + if coreCmd.Oem != "" {
> + packageQueue = append(packageQueue, coreCmd.Oem)
> + }
> + for k := range coreCmd.oem.Config {
> + // ubuntu-core is not a real package
> + if k != "ubuntu-core" {
What happens here? Is snappy just returning a error or is the world exploding? If the world explodes we might want to fix snappy too :)
> + packageQueue = append(packageQueue, k)
> + }
> + }
> + packageQueue = append(packageQueue, coreCmd.Install...)
> +
> + for _, snap := range packageQueue {
> + fmt.Println("Installing", snap)
>
> if err := snappy.Install(snap, flags); err != nil {
> return err
> @@ -493,7 +522,39 @@
> return string(pubKey), err
> }
>
> -func loadOem(systemPath string) (oem diskimage.OemDescription, err error) {
> +func (coreCmd *CoreCmd) extractOemDescription(oemPackage string) error {
> + if oemPackage == "" {
> + return nil
> + }
> +
> + tempDir, err := ioutil.TempDir("", "oem")
> + if err != nil {
> + return err
> + }
> + defer os.RemoveAll(tempDir)
> +
> + snappy.SetRootDir(tempDir)
Heh, clever!
> + defer snappy.SetRootDir("/")
> +
> + flags := snappy.InhibitHooks
> + if coreCmd.Development.DeveloperMode {
> + flags |= snappy.AllowUnauthenticated
> + }
> +
> + if err := snappy.Install(oemPackage, flags); err != nil {
> + return err
> + }
> +
> + oem, err := coreCmd.loadOem(tempDir)
> + if err != nil {
> + return err
> + }
> + coreCmd.oem = oem
> +
> + return nil
> +}
> +
> +func (coreCmd CoreCmd) loadOem(systemPath string) (oem diskimage.OemDescription, err error) {
> pkgs, err := filepath.Glob(filepath.Join(systemPath, "/oem/*/*/meta/package.yaml"))
> if err != nil {
> return oem, err
> @@ -515,6 +576,10 @@
> return oem, errors.New("cannot decode oem yaml")
> }
>
> + if oem.Platform() == "" {
> + oem.SetPlatform(coreCmd.Platform)
> + }
> +
> return oem, nil
> }
>
>
--
https://code.launchpad.net/~sergiusens/goget-ubuntu-touch/readOEMPackageYaml/+merge/254392
Your team Ubuntu Phablet Team is subscribed to branch lp:goget-ubuntu-touch.
More information about the Ubuntu-reviews
mailing list