[Merge] lp:~snappy-dev/goget-ubuntu-touch/all-snaps into lp:goget-ubuntu-touch
Sergio Schvezov
sergio.schvezov at canonical.com
Tue Jan 12 20:23:38 UTC 2016
Review: Needs Fixing
Thanks for this, it removes a bunch of legacy. I added a couple of inline comments, nothing major.
MPs like this https://code.launchpad.net/~sil2100/goget-ubuntu-touch/touch-devel-warning/+merge/282045 make me think we need to fork though since this will be expected to go all the way to trusty.
Diff comments:
>
> === modified file 'diskimage/common.go'
> --- diskimage/common.go 2015-10-26 18:30:45 +0000
> +++ diskimage/common.go 2016-01-12 11:31:26 +0000
> @@ -100,11 +100,11 @@
> RawFiles []BootAssetRawFiles `yaml:"raw-files,omitempty"`
> }
>
> -type OemDescription struct {
> +type GadgetDescription struct {
> Name string `yaml:"name"`
> Version string `yaml:"version"`
>
> - OEM struct {
> + GADGET struct {
Gadget (OEM is an acronym that's why it is all caps)
BTW, can't this now be taken from `snappy`?
> Hardware struct {
> Bootloader string `yaml:"bootloader"`
> PartitionLayout string `yaml:"partition-layout"`
> @@ -133,15 +133,15 @@
> rootDir string
> }
>
> -func (o *OemDescription) SetRoot(rootDir string) {
> +func (o *GadgetDescription) SetRoot(rootDir string) {
> o.rootDir = rootDir
> }
>
> // SystemParts returns the system labels depending on the partition layout.
> //
> // The default is to return a flat structure for any unknown layout.
> -func (o *OemDescription) SystemParts() []string {
> - switch o.OEM.Hardware.PartitionLayout {
> +func (o *GadgetDescription) SystemParts() []string {
> + switch o.GADGET.Hardware.PartitionLayout {
> case partLayoutSystemAB:
> return []string{"a", "b"}
we should get rid of partLayoutSystemAB if it isn't supported anymore too.
> default:
> @@ -259,8 +263,27 @@
> }
> img.baseMount = baseMount
>
> + mountpoints := make([]string, 0, len(bindMounts))
> + if img.gadget.PartitionLayout() == "minimal" {
> + mountpoints = bindMounts
> + }
> +
> + for _, d := range mountpoints {
why not put this loop inside the if?
> + p := filepath.Join(baseMount, d)
> +
> + if err := os.MkdirAll(p, 0755); err != nil {
this should not be needed on a readonly image, should it? It should of never have been needed, but oh well.. :-)
> + return err
> + }
> +
> + printOut("Bind mounting", d, "to", p)
> + if err := bindMount(filepath.Join("/", d), p); err != nil {
> + return err
> + }
> +
> + img.bindMounts = append(img.bindMounts, p)
> + }
> +
> return nil
> -
> }
>
> // Unmount unmounts the image. This also unmaps the loop device.
> @@ -459,49 +489,51 @@
>
> func (img *BaseImage) GenericBootSetup(bootPath string) error {
> // origins
> - hardwareYamlPath := filepath.Join(img.baseMount, hardwareFileName)
> - kernelPath := filepath.Join(img.baseMount, img.hardware.Kernel)
> - initrdPath := filepath.Join(img.baseMount, img.hardware.Initrd)
> -
> - // populate both A/B
> - for _, part := range img.oem.SystemParts() {
> - path := filepath.Join(bootPath, part)
> -
> - printOut("Setting up", path)
> -
> - if err := os.MkdirAll(path, 0755); err != nil {
> - return err
> - }
> -
> - if err := sysutils.CopyFile(hardwareYamlPath, filepath.Join(path, hardwareFileName)); err != nil {
> - return err
> - }
> -
> - if err := sysutils.CopyFile(kernelPath, filepath.Join(path, kernelFileName)); err != nil {
> - return err
> - }
> -
> - if err := sysutils.CopyFile(initrdPath, filepath.Join(path, initrdFileName)); err != nil {
> - return err
> + if img.gadget.PartitionLayout() == "system-AB" {
if we are removing support for this, just remove it IMO :-)
> + hardwareYamlPath := filepath.Join(img.baseMount, hardwareFileName)
> + kernelPath := filepath.Join(img.baseMount, img.hardware.Kernel)
> + initrdPath := filepath.Join(img.baseMount, img.hardware.Initrd)
> +
> + // populate both A/B
> + for _, part := range img.gadget.SystemParts() {
> + path := filepath.Join(bootPath, part)
> +
> + printOut("Setting up", path)
> +
> + if err := os.MkdirAll(path, 0755); err != nil {
> + return err
> + }
> +
> + if err := sysutils.CopyFile(hardwareYamlPath, filepath.Join(path, hardwareFileName)); err != nil {
> + return err
> + }
> +
> + if err := sysutils.CopyFile(kernelPath, filepath.Join(path, kernelFileName)); err != nil {
> + return err
> + }
> +
> + if err := sysutils.CopyFile(initrdPath, filepath.Join(path, initrdFileName)); err != nil {
> + return err
> + }
> }
> }
>
> - oemRoot, err := img.oem.InstallPath()
> + gadgetRoot, err := img.gadget.InstallPath()
> if err != nil {
> return err
> }
>
> - return setupBootAssetFiles(img.Boot(), bootPath, oemRoot, img.oem.OEM.Hardware.BootAssets.Files)
> + return setupBootAssetFiles(img.Boot(), bootPath, gadgetRoot, img.gadget.GADGET.Hardware.BootAssets.Files)
> }
>
> func (img *BaseImage) FlashExtra() error {
> - oemRoot, err := img.oem.InstallPath()
> + gadgetRoot, err := img.gadget.InstallPath()
> if err != nil {
> return err
> }
>
> - if bootAssets := img.oem.OEM.Hardware.BootAssets; bootAssets != nil {
> - return setupBootAssetRawFiles(img.location, oemRoot, bootAssets.RawFiles)
> + if bootAssets := img.gadget.GADGET.Hardware.BootAssets; bootAssets != nil {
> + return setupBootAssetRawFiles(img.location, gadgetRoot, bootAssets.RawFiles)
> }
>
> return nil
>
> === modified file 'diskimage/core_grub.go'
> --- diskimage/core_grub.go 2015-09-25 10:12:33 +0000
> +++ diskimage/core_grub.go 2016-01-12 11:31:26 +0000
> @@ -74,9 +86,20 @@
> }
>
> parted.addPart(grubLabel, "", fsNone, 4)
> +<<<<<<< TREE
merge conflict here
> parted.addPart(bootLabel, bootDir, fsFat32, 128)
> parted.addPart(systemALabel, systemADir, fsExt4, img.rootSize)
> parted.addPart(systemBLabel, systemBDir, fsExt4, img.rootSize)
> +=======
> + switch img.gadget.PartitionLayout() {
> + case "system-AB":
> + parted.addPart(bootLabel, bootDir, fsFat32, 64)
> + parted.addPart(systemALabel, systemADir, fsExt4, img.rootSize)
> + parted.addPart(systemBLabel, systemBDir, fsExt4, img.rootSize)
> + case "minimal":
> + parted.addPart(bootLabel, bootDir, fsFat32, 64)
> + }
> +>>>>>>> MERGE-SOURCE
> parted.addPart(writableLabel, writableDir, fsExt4, -1)
>
> parted.setBoot(2)
>
> === modified file 'diskimage/core_uboot.go'
> --- diskimage/core_uboot.go 2015-09-25 10:12:19 +0000
> +++ diskimage/core_uboot.go 2016-01-12 11:31:26 +0000
> @@ -66,15 +66,23 @@
> Bootloader []string `yaml:"bootloader"`
> }
>
> -func NewCoreUBootImage(location string, size int64, rootSize int, hw HardwareDescription, oem OemDescription) *CoreUBootImage {
> +func NewCoreUBootImage(location string, size int64, rootSize int, hw HardwareDescription, gadget GadgetDescription) *CoreUBootImage {
> + var partCount int
> + switch gadget.PartitionLayout() {
> + case "system-AB":
remove?
> + partCount = 4
> + case "minimal":
> + partCount = 2
> + }
> +
> return &CoreUBootImage{
> BaseImage{
> hardware: hw,
> - oem: oem,
> + gadget: gadget,
> location: location,
> size: size,
> rootSize: rootSize,
> - partCount: 4,
> + partCount: partCount,
> },
> }
> }
> @@ -90,9 +98,20 @@
> return err
> }
>
> +<<<<<<< TREE
merge conflict
> parted.addPart(bootLabel, bootDir, fsFat32, 128)
> parted.addPart(systemALabel, systemADir, fsExt4, 1024)
> parted.addPart(systemBLabel, systemBDir, fsExt4, 1024)
> +=======
> + switch img.gadget.PartitionLayout() {
> + case "system-AB":
> + parted.addPart(bootLabel, bootDir, fsFat32, 64)
> + parted.addPart(systemALabel, systemADir, fsExt4, 1024)
> + parted.addPart(systemBLabel, systemBDir, fsExt4, 1024)
> + case "minimal":
> + parted.addPart(bootLabel, bootDir, fsFat32, 128)
> + }
> +>>>>>>> MERGE-SOURCE
> parted.addPart(writableLabel, writableDir, fsExt4, -1)
>
> parted.setBoot(1)
> @@ -149,28 +168,32 @@
> return err
> }
>
> - dtbFis, err := ioutil.ReadDir(dtbsPath)
> - if err != nil {
> - return err
> + var dtbFis []os.FileInfo
> + if img.hardware.Dtbs != "" {
> + var err error
> + dtbFis, err = ioutil.ReadDir(dtbsPath)
> + if err != nil {
> + return err
> + }
> }
>
> if err := os.MkdirAll(bootDtbPath, 0755); err != nil {
> return err
> }
>
> - dtb := filepath.Join(dtbsPath, fmt.Sprintf("%s.dtb", img.oem.Platform()))
> + dtb := filepath.Join(dtbsPath, fmt.Sprintf("%s.dtb", img.gadget.Platform()))
>
> // if there is a specific dtb for the platform, copy it.
> - // First look in oem and then in device.
> - if oemDtb := img.oem.OEM.Hardware.Dtb; oemDtb != "" && img.oem.Platform() != "" {
> - oemRoot, err := img.oem.InstallPath()
> + // First look in gadget and then in device.
> + if gadgetDtb := img.gadget.GADGET.Hardware.Dtb; gadgetDtb != "" && img.gadget.Platform() != "" {
weren't we going to carry the dtb in one location only from now on? honest question
> + gadgetRoot, err := img.gadget.InstallPath()
> if err != nil {
> return err
> }
>
> - oemDtb := filepath.Join(oemRoot, oemDtb)
> + gadgetDtb := filepath.Join(gadgetRoot, gadgetDtb)
> dst := filepath.Join(bootDtbPath, filepath.Base(dtb))
> - if err := sysutils.CopyFile(oemDtb, dst); err != nil {
> + if err := sysutils.CopyFile(gadgetDtb, dst); err != nil {
> return err
> }
> } else if _, err := os.Stat(dtb); err == nil {
>
> === modified file 'ubuntu-device-flash/snappy.go'
> --- ubuntu-device-flash/snappy.go 2015-11-12 16:48:10 +0000
> +++ ubuntu-device-flash/snappy.go 2016-01-12 11:31:26 +0000
> @@ -187,15 +201,68 @@
> }
> }
>
> + // set the bootvars for kernel/os snaps, the latest snappy is
> + // not activating the snaps on install anymore (with inhibit)
> + // so we need to work around that here (only on first boot)
> + //
> + // there is also no mounted os/kernel snap in the systemPath
> + // all we have here is the blobs
> + if s.OS != "" && s.Kernel != "" {
> + snaps, _ := filepath.Glob(filepath.Join(dirs.SnapBlobDir, "*.snap"))
> + for _, fullname := range snaps {
> + bootvar := ""
> +
> + // detect type
> + snapFile, err := snap.Open(fullname)
> + if err != nil {
> + return fmt.Errorf("can not read %v", fullname)
> + }
> + info, err := snapFile.Info()
> + if err != nil {
> + return fmt.Errorf("can not get info for %v", fullname)
> + }
> + switch info.Type {
> + case snap.TypeOS:
> + bootvar = "snappy_os"
> + case snap.TypeKernel:
> + bootvar = "snappy_kernel"
> + }
> +
> + name := filepath.Base(fullname)
> + if bootvar != "" {
> + if err := partition.SetBootVar(bootvar, name); err != nil {
> + return err
> + }
> + }
> + }
> +
> + // HORRIBLE, snappy.Install() will check if running
I say we just standarize and duplicate the code.
> + // on a grub system based on the gadget snap and if
> + // it is grub it will not extract the kernel/os
> + //
> + // HOWEVER this won't work in u-d-f because there
> + // is no current symlink so kernel.go always unpacks
> + // the kernel. undo this here
> + if s.gadget.GADGET.Hardware.Bootloader == "grub" {
> + dirs, _ := filepath.Glob(filepath.Join(s.img.Boot(), "/EFI/ubuntu/grub/*.snap"))
> + for _, d := range dirs {
> + fmt.Printf("Removing unneeded: %s\n", d)
> + if err := os.RemoveAll(d); err != nil {
> + return err
> + }
> + }
> + }
> + }
> +
> return nil
> }
>
> -func (s *Snapper) extractOem(oemPackage string) error {
> - if oemPackage == "" {
> +func (s *Snapper) extractGadget(gadgetPackage string) error {
> + if gadgetPackage == "" {
> return nil
> }
>
> - tempDir, err := ioutil.TempDir("", "oem")
> + tempDir, err := ioutil.TempDir("", "gadget")
> if err != nil {
> return err
> }
> @@ -215,43 +282,66 @@
> Channel: s.Channel,
> })
>
> - flags := s.installFlags()
> - pb := progress.NewTextProgress()
> - if _, err := snappy.Install(oemPackage, flags, pb); err != nil {
> + // we need to download and extrac the squashfs snap
extract
alternatively, you could just mount it too (although that requires root and might be more cumbersome).
> + downloadedSnap := gadgetPackage
> + if !helpers.FileExists(gadgetPackage) {
> + repo := snappy.NewUbuntuStoreSnapRepository()
> + snaps, err := repo.Details(snappy.SplitOrigin(gadgetPackage))
> + if len(snaps) != 1 {
> + return fmt.Errorf("expected 1 gadget snaps, found %d", len(snaps))
> + }
> +
> + pb := progress.NewTextProgress()
> + downloadedSnap, err = snaps[0].(*snappy.RemoteSnapPart).Download(pb)
> + if err != nil {
> + return err
> + }
> + }
> +
> + // the
the.... end? :-)
> + fakeGadgetDir := filepath.Join(tempDir, "/gadget/fake-gadget/1.0/")
not so fond of the 1.0 here... maybe 1.0-fake?
> + if err := os.MkdirAll(fakeGadgetDir, 0755); err != nil {
> return err
> }
> + cmd := exec.Command("unsquashfs", "-f", "-d", fakeGadgetDir, downloadedSnap)
> + if output, err := cmd.CombinedOutput(); err != nil {
> + return fmt.Errorf("snap unpack failed with: %v (%v)", err, string(output))
> + }
>
> - if err := s.loadOem(tempDir); err != nil {
> + if err := s.loadGadget(tempDir); err != nil {
> return err
> }
>
> return nil
> }
>
> -func (s *Snapper) loadOem(systemPath string) error {
> - pkgs, err := filepath.Glob(filepath.Join(systemPath, "/oem/*/*/meta/package.yaml"))
> +func (s *Snapper) loadGadget(systemPath string) error {
> + pkgs, err := filepath.Glob(filepath.Join(systemPath, "/gadget/*/*/meta/package.yaml"))
> if err != nil {
> return err
> }
>
> // checking for len(pkgs) > 2 due to the 'current' symlink
> if len(pkgs) == 0 {
> - return errors.New("no oem package found")
> + return errors.New("no gadget package found")
> } else if len(pkgs) > 2 || err != nil {
> - return errors.New("too many oem packages installed")
> + return errors.New("too many gadget packages installed")
> }
>
> f, err := ioutil.ReadFile(pkgs[0])
> if err != nil {
> - return errors.New("failed to read oem yaml")
> - }
> -
> - var oem diskimage.OemDescription
> - if err := yaml.Unmarshal([]byte(f), &oem); err != nil {
> - return errors.New("cannot decode oem yaml")
> - }
> - s.oem = oem
> - s.oem.SetRoot(systemPath)
> + return errors.New("failed to read gadget yaml")
> + }
> +
> + var gadget diskimage.GadgetDescription
> + if err := yaml.Unmarshal([]byte(f), &gadget); err != nil {
> + return errors.New("cannot decode gadget yaml")
> + }
> + s.gadget = gadget
> + s.gadget.SetRoot(systemPath)
> +
> + // ensure we can install snaps
and download?
> + arch.SetArchitecture(arch.ArchitectureType(s.gadget.Architecture()))
>
> return nil
> }
> @@ -515,19 +724,58 @@
>
> s.hardware = <-hwChan
>
> - loader := s.oem.OEM.Hardware.Bootloader
> - switch loader {
> + return filePaths, nil
> +}
> +
> +func (s *Snapper) create() (err error) {
> + if err := s.sanityCheck(); err != nil {
> + return err
> + }
> +
> + if s.StoreID != "" {
> + fmt.Println("Setting store id to", s.StoreID)
> + os.Setenv("UBUNTU_STORE_ID", s.StoreID)
> + }
> +
> + fmt.Println("Determining gadget configuration")
> + if err := s.extractGadget(s.Gadget); err != nil {
> + return err
> + }
> + defer os.RemoveAll(s.stagingRootPath)
> +
> + // hack to circumvent https://code.google.com/p/go/issues/detail?id=1435
> + runtime.GOMAXPROCS(1)
> + runtime.LockOSThread()
> + if err := sysutils.DropPrivs(); err != nil {
> + return err
> + }
> +
> + systemImageFiles := []Files{}
> + switch s.gadget.GADGET.Hardware.PartitionLayout {
> + case "system-AB":
> + si, err := s.getSystemImage()
> + if err != nil {
> + return err
> + }
> + systemImageFiles = si
> + case "minimal":
> + if s.OS == "" && s.Kernel == "" {
> + return errors.New("kernel and os have to be specified to support partition-layout: minimal")
> + }
> + }
> +
> + switch s.gadget.GADGET.Hardware.Bootloader {
> case "grub":
> legacy := isLegacy(s.Positional.Release, s.Channel, globalArgs.Revision)
> if legacy {
> printOut("Using legacy setup")
> }
>
> - s.img = diskimage.NewCoreGrubImage(s.Output, s.size, s.flavor.rootSize(), s.hardware, s.oem, legacy)
> + s.img = diskimage.NewCoreGrubImage(s.Output, s.size, s.flavor.rootSize(), s.hardware, s.gadget, legacy)
> case "u-boot":
> - s.img = diskimage.NewCoreUBootImage(s.Output, s.size, s.flavor.rootSize(), s.hardware, s.oem)
> + s.img = diskimage.NewCoreUBootImage(s.Output, s.size, s.flavor.rootSize(), s.hardware, s.gadget)
> default:
> - return errors.New("no hardware description in OEM snap")
> + return errors.New("no hardware description in GADGET snap")
gadget
> }
>
> printOut("Partitioning...")
--
https://code.launchpad.net/~snappy-dev/goget-ubuntu-touch/all-snaps/+merge/275273
Your team Ubuntu Phablet Team is subscribed to branch lp:goget-ubuntu-touch.
More information about the Ubuntu-reviews
mailing list