Merge lp:~chipaca/snappy/deprecated-architectures into lp:~snappy-dev/snappy/snappy-moved-to-github

[John Lenton]

Improved parsing of deprecated "architecture" entry of package.yaml.

Currently, the deprecated "architecture" entry in package.yaml is loaded as an interface{}, and only some of the values therein are considered.

I've had a bit of fun loading arbitrarily large data structures into that before I got bored and fixed it.

Two silly examples of things that should've been failing with an error but weren't are included as tests. One leaves the packageYaml struct with a nil 'Architectures' field (which goes on to cause a panic during build); the other panics in the parser itself. You get to guess which one did which (or you can revert back to the first commit, which is just the tests).

The panic during build is because debArchitecture doesn't check bounds. Not sure what it can do if Architecture is empty on the package, though; have gone with returning "unknown", but maybe we _want_ it to panic there.

Way-too-much-fun-on-the-train-home-ly yours,

[Michael Vogt (mvo)]

Thanks for fixing this! Code looks great, one question inline.

review: Approve

[Michael Vogt (mvo)]

Hm, setting to needs information just to be sure that the inline question is not forgotten :)

review: Needs Information

[Sergio Schvezov (sergiusens)]

looks good and clean, just one comment

[Sergio Schvezov (sergiusens)]

nevermind my flaky eyes!

review: Approve

=== modified file 'snappy/build.go'
--- snappy/build.go	2015-05-02 22:53:19 +0000
+++ snappy/build.go	2015-05-02 22:53:19 +0000
@@ -83,10 +83,14 @@
 
 // small helper that return the architecture or "multi" if its multiple arches
 func debArchitecture(m *packageYaml) string {
-	if len(m.Architectures) > 1 {
+	switch len(m.Architectures) {
+	case 0:
+		return "unknown"
+	case 1:
+		return m.Architectures[0]
+	default:
 		return "multi"
 	}
-	return m.Architectures[0]
 }
 
 func parseReadme(readme string) (title, description string, err error) {

=== modified file 'snappy/build_test.go'
--- snappy/build_test.go	2015-04-16 16:30:26 +0000
+++ snappy/build_test.go	2015-05-02 22:53:19 +0000
@@ -403,3 +403,9 @@
 	_, err := Build(sourceDir, "")
 	c.Assert(err, ErrorMatches, ".*binary and service both called foo.*")
 }
+
+func (s *SnapTestSuite) TestDebArchitecture(c *C) {
+	c.Check(debArchitecture(&packageYaml{Architectures: []string{"foo"}}), Equals, "foo")
+	c.Check(debArchitecture(&packageYaml{Architectures: []string{"foo", "bar"}}), Equals, "multi")
+	c.Check(debArchitecture(&packageYaml{Architectures: nil}), Equals, "unknown")
+}

=== modified file 'snappy/snapp.go'
--- snappy/snapp.go	2015-05-01 12:32:42 +0000
+++ snappy/snapp.go	2015-05-02 22:53:19 +0000
@@ -172,6 +172,26 @@
 
 var commasplitter = regexp.MustCompile(`\s*,\s*`).Split
 
+// deprecarch handles the vagaries of the now-deprecated
+// "architecture" field of the package.yaml
+type deprecarch []string
+
+func (v *deprecarch) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	var l []string
+
+	if err := unmarshal(&l); err != nil {
+		var s string
+		if err := unmarshal(&s); err != nil {
+			return err
+
+		}
+		l = append([]string(nil), s)
+	}
+	*v = deprecarch(l)
+
+	return nil
+}
+
 // TODO split into payloads per package type composing the common
 // elements for all snaps.
 type packageYaml struct {
@@ -183,8 +203,8 @@
 
 	// the spec allows a string or a list here *ick* so we need
 	// to convert that into something sensible via reflect
-	DeprecatedArchitecture interface{} `yaml:"architecture"`
-	Architectures          []string    `yaml:"architectures"`
+	DeprecatedArchitecture deprecarch `yaml:"architecture"`
+	Architectures          []string   `yaml:"architectures"`
 
 	DeprecatedFramework string   `yaml:"framework,omitempty"`
 	Frameworks          []string `yaml:"frameworks,omitempty"`
@@ -247,20 +267,11 @@
 		return nil, err
 	}
 
-	// parse the architecture: field that is either a string or a list
-	// or empty (yes, you read that correctly)
 	if m.Architectures == nil {
-		v := reflect.ValueOf(m.DeprecatedArchitecture)
-		switch v.Kind() {
-		case reflect.Invalid:
+		if m.DeprecatedArchitecture == nil {
 			m.Architectures = []string{"all"}
-		case reflect.String:
-			m.Architectures = []string{v.String()}
-		case reflect.Slice:
-			v2 := m.DeprecatedArchitecture.([]interface{})
-			for _, arch := range v2 {
-				m.Architectures = append(m.Architectures, arch.(string))
-			}
+		} else {
+			m.Architectures = m.DeprecatedArchitecture
 		}
 	}
 

=== modified file 'snappy/snapp_test.go'
--- snappy/snapp_test.go	2015-04-22 09:06:36 +0000
+++ snappy/snapp_test.go	2015-05-02 22:53:19 +0000
@@ -801,6 +801,30 @@
 	c.Assert(m.Architectures, DeepEquals, []string{"all"})
 }
 
+func (s *SnapTestSuite) TestPackageYamlBadArchitectureParsing(c *C) {
+	data := []byte(`name: fatbinary
+version: 1.0
+vendor: Michael Vogt <[e-mail]>
+architecture:
+  armhf:
+    no
+`)
+	_, err := parsePackageYamlData(data)
+	c.Assert(err, NotNil)
+}
+
+func (s *SnapTestSuite) TestPackageYamlWorseArchitectureParsing(c *C) {
+	data := []byte(`name: fatbinary
+version: 1.0
+vendor: Michael Vogt <[e-mail]>
+architecture:
+  - armhf:
+      sometimes
+`)
+	_, err := parsePackageYamlData(data)
+	c.Assert(err, NotNil)
+}
+
 func (s *SnapTestSuite) TestPackageYamlLicenseParsing(c *C) {
 	y := filepath.Join(s.tempdir, "package.yaml")
 	ioutil.WriteFile(y, []byte(`explicit-license-agreement: Y`), 0644)