[Merge] lp:~sergiusens/goget-ubuntu-touch/oemDtb into lp:goget-ubuntu-touch
Michael Vogt
michael.vogt at canonical.com
Thu Jan 29 14:13:05 UTC 2015
Review: Approve
Looks good, you could filter /current/ too but I guess its fine.
Diff comments:
> === modified file 'diskimage/common.go'
> --- diskimage/common.go 2015-01-15 21:05:01 +0000
> +++ diskimage/common.go 2015-01-28 19:40:41 +0000
> @@ -11,6 +11,7 @@
> "fmt"
> "os"
> "os/exec"
> + "path/filepath"
> "strings"
> )
>
> @@ -52,7 +53,7 @@
> type CoreImage interface {
> Image
> SystemImage
> - SetupBoot() error
> + SetupBoot(OemDescription) error
> FlashExtra(string) error
> }
>
> @@ -64,6 +65,18 @@
> Bootloader string `yaml:"bootloader"`
> }
>
> +type OemDescription struct {
> + Name string `yaml:"name"`
> + Version string `yaml:"version"`
> + Hardware struct {
> + Dtb string `yaml:"dtb,omitempty"`
> + } `yaml:"hardware,omitempty"`
> +}
> +
> +func (o OemDescription) InstallPath() string {
> + return filepath.Join("/oem", o.Name, o.Version)
> +}
> +
> 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-21 23:22:53 +0000
> +++ diskimage/core_grub.go 2015-01-28 19:40:41 +0000
> @@ -268,7 +268,7 @@
> return img.baseMount
> }
>
> -func (img *CoreGrubImage) SetupBoot() error {
> +func (img *CoreGrubImage) SetupBoot(oem OemDescription) 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-01-28 13:04:27 +0000
> +++ diskimage/core_uboot.go 2015-01-28 19:40:41 +0000
> @@ -290,7 +290,7 @@
> return img.baseMount
> }
>
> -func (img CoreUBootImage) SetupBoot() error {
> +func (img CoreUBootImage) SetupBoot(oem OemDescription) error {
> // destinations
> bootPath := filepath.Join(img.baseMount, string(bootDir))
> bootAPath := filepath.Join(bootPath, "a")
> @@ -313,19 +313,19 @@
> return err
> }
>
> - if err := move(hardwareYamlPath, filepath.Join(bootAPath, "hardware.yaml")); err != nil {
> - return err
> - }
> -
> - if err := move(kernelPath, filepath.Join(bootAPath, filepath.Base(kernelPath))); err != nil {
> - return err
> - }
> -
> - if err := move(initrdPath, filepath.Join(bootAPath, filepath.Base(initrdPath))); err != nil {
> - return err
> - }
> -
> - if err := img.provisionDtbs(bootDtbPath); err != nil {
> + if err := copyFile(hardwareYamlPath, filepath.Join(bootAPath, "hardware.yaml")); err != nil {
> + return err
> + }
> +
> + if err := copyFile(kernelPath, filepath.Join(bootAPath, filepath.Base(kernelPath))); err != nil {
> + return err
> + }
> +
> + if err := copyFile(initrdPath, filepath.Join(bootAPath, filepath.Base(initrdPath))); err != nil {
> + return err
> + }
> +
> + if err := img.provisionDtbs(oem, bootDtbPath); err != nil {
> return err
> }
>
> @@ -374,7 +374,7 @@
> // if a uEnv.txt is provided in the flashtool-assets, use it
> if _, err := os.Stat(uEnvPath); err == nil {
> printOut("Adding uEnv.txt to", bootuEnvPath)
> - if err := move(uEnvPath, bootuEnvPath); err != nil {
> + if err := copyFile(uEnvPath, bootuEnvPath); err != nil {
> return err
> }
> } else {
> @@ -384,7 +384,7 @@
> return nil
> }
>
> -func (img CoreUBootImage) provisionDtbs(bootDtbPath string) error {
> +func (img CoreUBootImage) provisionDtbs(oem OemDescription, bootDtbPath string) error {
> dtbsPath := filepath.Join(img.baseMount, img.hardware.Dtbs)
>
> if _, err := os.Stat(dtbsPath); os.IsNotExist(err) {
> @@ -401,16 +401,23 @@
> dtb := filepath.Join(dtbsPath, fmt.Sprintf("%s.dtb", img.platform))
>
> // if there is a specific dtb for the platform, copy it.
> - if _, err := os.Stat(dtb); err == nil {
> - dst := filepath.Join(bootDtbPath, filepath.Base(dtb))
> - if err := move(dtb, dst); err != nil {
> + // First look in oem and then in device.
> + if oem.Hardware.Dtb != "" && img.platform != "" {
> + oemDtb := filepath.Join(img.System(), oem.InstallPath(), oem.Hardware.Dtb)
> + dst := filepath.Join(bootDtbPath, filepath.Base(dtb))
> + if err := copyFile(oemDtb, dst); err != nil {
> + return err
> + }
> + } else if _, err := os.Stat(dtb); err == nil {
> + dst := filepath.Join(bootDtbPath, filepath.Base(dtb))
> + if err := copyFile(dtb, dst); err != nil {
> return err
> }
> } else {
> for _, dtbFi := range dtbFis {
> src := filepath.Join(dtbsPath, dtbFi.Name())
> dst := filepath.Join(bootDtbPath, dtbFi.Name())
> - if err := move(src, dst); err != nil {
> + if err := copyFile(src, dst); err != nil {
> return err
> }
> }
> @@ -475,7 +482,7 @@
> return nil
> }
>
> -func move(src, dst string) error {
> +func copyFile(src, dst string) error {
I'm just trolling golang a bit - I think it would be a good idea to have standard functions for common code like CopyFile. IMO its a waste of develop time if this needs to be reimplemented N times (with N getting pretty big as the popularity grows). But oh well, not your fault :)
> dstFile, err := os.Create(dst)
> if err != nil {
> return err
> @@ -486,7 +493,6 @@
> if err != nil {
> return err
> }
> - defer os.Remove(src)
> defer srcFile.Close()
>
> reader := bufio.NewReader(srcFile)
>
> === modified file 'ubuntu-device-flash/core.go'
> --- ubuntu-device-flash/core.go 2015-01-19 21:38:25 +0000
> +++ ubuntu-device-flash/core.go 2015-01-28 19:40:41 +0000
> @@ -319,7 +319,17 @@
>
> systemPath := img.System()
>
> - if err := img.SetupBoot(); err != nil {
> + if err := coreCmd.install(systemPath); err != nil {
> + 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 {
> return err
> }
>
> @@ -327,10 +337,6 @@
> return err
> }
>
> - if err := coreCmd.install(systemPath); err != nil {
> - return err
> - }
> -
> if !coreCmd.Cloud {
> cloudBaseDir := filepath.Join("var", "lib", "cloud")
>
> @@ -494,6 +500,42 @@
> return string(pubKey), err
> }
>
> +func loadOem(systemPath string) (oem diskimage.OemDescription, err error) {
> + pkgs, err := glob(filepath.Join(systemPath, "/oem/"), "package.yaml")
Indeed, good point about "current", you could filter by "/apps/*/[0-9-.]*" or just skip "/current/" if you find it in the list.
> + if err != nil {
> + return oem, err
> + }
> +
> + if len(pkgs) == 0 {
> + return oem, nil
> + } else if len(pkgs) > 1 || err != nil {
> + return oem, errors.New("too many oem packages installed")
> + }
> +
> + f, err := ioutil.ReadFile(pkgs[0])
> + if err != nil {
> + return oem, errors.New("failed to read oem yaml")
> + }
> +
> + if err := goyaml.Unmarshal([]byte(f), &oem); err != nil {
> + return oem, errors.New("cannot decode oem yaml")
> + }
> +
> + return oem, nil
> +}
> +
> +func glob(dir string, filename string) (pkgs []string, err error) {
> + err = filepath.Walk(dir, func(path string, f os.FileInfo, err error) error {
> + if filepath.Base(path) == filename {
> + pkgs = append(pkgs, path)
> + }
> +
> + return nil
> + })
> +
> + return pkgs, err
> +}
> +
> func extractHWDescription(path string) (hw diskimage.HardwareDescription, err error) {
> // hack to circumvent https://code.google.com/p/go/issues/detail?id=1435
> if syscall.Getuid() == 0 {
>
--
https://code.launchpad.net/~sergiusens/goget-ubuntu-touch/oemDtb/+merge/247886
Your team Ubuntu Phablet Team is subscribed to branch lp:goget-ubuntu-touch.
More information about the Ubuntu-reviews
mailing list