Marshaler Should not writeQuoted string #341
Labels
Comments
|
I think this is because I copied this by accident from the already existing TextMarshaler. You should be able to write |
Hi, did you had any chance to took at this issue. I'll share here some code to test it https://go.dev/play/p/WcKntwa2rze See, I've compared to json marshal to illustrate the expected behaviour Just run it and you will see what is wrong. MarshalTOML result should no be wrote with quotes. But please confirm |
No, sorry; I haven't had the time. Not sure if I have in the coming weeks, but I'll be happy to review and merge patches in a timely fashion. |
Thanks a lot! Feel free to suggest any changes or ask for extra clarification if required. My main concern is that this could affect previous implementations of MarshalTOML |
DarrylWong
pushed a commit
to DarrylWong/go-benchmarks
that referenced
this issue
Apr 22, 2024
Change has been successfully cherry-picked as 1bae5b3 3 is the latest approved patch-set. The change was submitted with unreviewed changes in the following files: ``` The name of the file: sweet/cmd/sweet/integration_test.go Insertions: 70, Deletions: 51. @@ -15,7 +15,6 @@ "sync" "testing" - "github.com/BurntSushi/toml" "golang.org/x/benchmarks/sweet/common" "golang.org/x/sync/semaphore" ) @@ -24,6 +23,9 @@ if runtime.GOOS != "linux" || runtime.GOARCH != "amd64" { t.Skip("Sweet is currently only fully supported on linux/amd64") } + if testing.Short() { + t.Skip("the full Sweet end-to-end experience takes several minutes") + } goRoot := os.Getenv("GOROOT") if goRoot == "" { data, err := exec.Command("go", "env", "GOROOT").Output() @@ -47,42 +49,53 @@ if err := goTool.BuildPath(filepath.Join(sweetRoot, "cmd", "sweet"), sweetBin); err != nil { t.Fatal(err) } - tmpDir, err := os.MkdirTemp("", "example") - if err != nil { - t.Fatal(err) + // We're on a builder, so arrange all this a little differently. + // Let's do all our work in the work directory which has a lot + // more headroom, and put the compressed assets in /tmp. + var tmpDir, assetsCacheDir string + if os.Getenv("GO_BUILDER_NAME") != "" { + tmpDir = filepath.Join(sweetRoot, "tmp") + if err := os.Mkdir(tmpDir, 0777); err != nil { + t.Fatal(err) + } + // Be explicit that we want /tmp, because the builder is + // going to try and give us /workdir/tmp which will not + // have enough space for us. + assetsCacheDir = filepath.Join("/", "tmp", "go-sweet-assets") + defer func() { + if err := os.RemoveAll(assetsCacheDir); err != nil { + t.Errorf("clearing assets cache directory: %v", err) + } + }() + } else { + tmpDir, err = os.MkdirTemp("", "go-sweet-test") + if err != nil { + t.Fatal(err) + } + assetsCacheDir = filepath.Join(tmpDir, "assets") } - assetsDir := filepath.Join(tmpDir, "assets") + defer func() { + if err := os.RemoveAll(tmpDir); err != nil { + t.Errorf("clearing tmp directory: %v", err) + } + }() // Download assets. - t.Run("Get", func(t *testing.T) { - getCmd := exec.Command(sweetBin, "get", - "-auth", "app-default", - "-cache", "", // Make a full copy so we can mutate it. - "-assets-hash-file", filepath.Join(sweetRoot, "assets.hash"), - "-assets-dir", assetsDir, - ) - if output, err := getCmd.CombinedOutput(); err != nil { - t.Logf("command output:\n%s", string(output)) - t.Fatal(err) - } - }) + getCmd := exec.Command(sweetBin, "get", + "-auth", "none", + "-cache", assetsCacheDir, // Make a full copy so we can mutate it. + "-assets-hash-file", filepath.Join(sweetRoot, "assets.hash"), + ) + if output, err := getCmd.CombinedOutput(); err != nil { + t.Logf("command output:\n%s", string(output)) + t.Fatal(err) + } - // Regenerate assets. - sourceAssetsDir := filepath.Join(sweetRoot, "source-assets") - - t.Run("Gen", func(t *testing.T) { - genCmd := exec.Command(sweetBin, "gen", - // Only do source assets. It takes far too long to do all assets. - "-gen", "gvisor,gopher-lua", - "-assets-dir", assetsDir, - "-source-assets-dir", sourceAssetsDir, - "-output-dir", assetsDir, - ) - if output, err := genCmd.CombinedOutput(); err != nil { - t.Logf("command output:\n%s", string(output)) - t.Fatal(err) - } - }) + // TODO(mknyszek): Test regenerating assets. As it stands, the following + // parts of the test will fail if the source assets change, since they're + // prebuilt and baked into the assets archive. The only recourse is to + // first upload the new archive with the prebuilt assets (i.e. run sweet + // gen locally), bump the version, and then upload it (i.e. sweet put). // Run each benchmark once. benchDir := filepath.Join(sweetRoot, "benchmarks") @@ -94,7 +107,7 @@ "-run", shard, "-shell", "-count", "1", - "-assets-dir", assetsDir, + "-cache", assetsCacheDir, "-bench-dir", benchDir, "-results", resultsDir, "-work-dir", workDir, @@ -138,22 +151,24 @@ t.Error(runErr) } } - t.Run("RunAll", func(t *testing.T) { - // Limit parallelism to conserve memory. - sema := semaphore.NewWeighted(2) - for i, shard := range []string{ - "tile38", "go-build", "biogo-igor", "biogo-krishna", "bleve-query", - "gvisor", "fogleman-pt", "bleve-index,fogleman-fauxgl,gopher-lua,markdown", - } { - sema.Acquire(context.Background(), 1) - go func(i int, shard string) { - defer sema.Release(1) - resultsDir := filepath.Join(tmpDir, fmt.Sprintf("results-%d", i)) - workDir := filepath.Join(tmpDir, fmt.Sprintf("tmp-%d", i)) - runShard(shard, resultsDir, workDir) - }(i, shard) - } - }) + // Limit parallelism to conserve memory. + sema := semaphore.NewWeighted(2) + var wg sync.WaitGroup + for i, shard := range []string{ + "tile38", "go-build", "biogo-igor", "biogo-krishna", "bleve-query", + "gvisor", "fogleman-pt", "bleve-index,fogleman-fauxgl,gopher-lua,markdown", + } { + sema.Acquire(context.Background(), 1) + wg.Add(1) + go func(i int, shard string) { + defer sema.Release(1) + defer wg.Done() + resultsDir := filepath.Join(tmpDir, fmt.Sprintf("results-%d", i)) + workDir := filepath.Join(tmpDir, fmt.Sprintf("tmp-%d", i)) + runShard(shard, resultsDir, workDir) + }(i, shard) + } + wg.Wait() } func makeConfigFile(t *testing.T, goRoot string) string { @@ -172,7 +187,11 @@ }, }, } - if err := toml.NewEncoder(f).Encode(&cfg); err != nil { + b, err := common.ConfigFileMarshalTOML(&cfg) + if err != nil { + t.Fatal(err) + } + if _, err := f.Write(b); err != nil { t.Fatal(err) } return f.Name() ``` ``` The name of the file: sweet/common/config.go Insertions: 37, Deletions: 1. @@ -5,9 +5,11 @@ package common import ( + "bytes" "fmt" "path/filepath" - "strings" + + "github.com/BurntSushi/toml" ) const ConfigHelp = ` @@ -54,22 +56,45 @@ } } +func ConfigFileMarshalTOML(c *ConfigFile) ([]byte, error) { + // Unfortunately because the github.com/BurntSushi/toml + // package at v1.0.0 doesn't correctly support Marshaler + // (see BurntSushi/toml#341) + // we can't actually implement Marshaler for ConfigEnv. + // So instead we work around this by implementing MarshalTOML + // on Config and use dummy types that have a straightforward + // mapping that *does* work. + type config struct { + Name string `toml:"name"` + GoRoot string `toml:"goroot"` + BuildEnv []string `toml:"envbuild"` + ExecEnv []string `toml:"envexec"` + } + type configFile struct { + Configs []*config `toml:"config"` + } + var cfgs configFile + for _, c := range c.Configs { + var cfg config + cfg.Name = c.Name + cfg.GoRoot = c.GoRoot + cfg.BuildEnv = c.BuildEnv.Collapse() + cfg.ExecEnv = c.ExecEnv.Collapse() + + cfgs.Configs = append(cfgs.Configs, &cfg) + } + var b bytes.Buffer + if err := toml.NewEncoder(&b).Encode(&cfgs); err != nil { + return nil, err + } + return b.Bytes(), nil +} + type ConfigEnv struct { *Env } -func (c *ConfigEnv) MarshalText() ([]byte, error) { - if c.Env == nil { - return []byte("[]"), nil - } - envs := c.Collapse() - for i, env := range envs { - envs[i] = "\"" + env + "\"" - } - return []byte("[" + strings.Join(envs, ", ") + "]"), nil -} - -func (c *ConfigEnv) UnmarshalText(data interface{}) error { +func (c *ConfigEnv) UnmarshalTOML(data interface{}) error { ldata, ok := data.([]interface{}) if !ok { return fmt.Errorf("expected data for env to be a list") ``` ``` The name of the file: sweet/common/config_test.go Insertions: 83, Deletions: 0. @@ -0,0 +1,83 @@ +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package common_test + +import ( + "strings" + "testing" + + "github.com/BurntSushi/toml" + "golang.org/x/benchmarks/sweet/common" +) + +func TestConfigMarshalTOML(t *testing.T) { + cfgsBefore := common.ConfigFile{ + Configs: []*common.Config{ + &common.Config{ + Name: "go", + GoRoot: "/path/to/my/goroot", + // The unmarashaler propagates the environment, + // so to make sure this works, let's also seed + // from the environment. + BuildEnv: common.ConfigEnv{common.NewEnvFromEnviron()}, + ExecEnv: common.ConfigEnv{common.NewEnvFromEnviron()}, + }, + }, + } + b, err := common.ConfigFileMarshalTOML(&cfgsBefore) + if err != nil { + t.Fatal(err) + } + var cfgsAfter common.ConfigFile + if err := toml.Unmarshal(b, &cfgsAfter); err != nil { + t.Fatal(err) + } + if l := len(cfgsAfter.Configs); l != len(cfgsBefore.Configs) { + t.Fatalf("unexpected number of configs: got %d, want %d", l, len(cfgsBefore.Configs)) + } + for i := range cfgsAfter.Configs { + cfgBefore := cfgsBefore.Configs[i] + cfgAfter := cfgsAfter.Configs[i] + + if cfgBefore.Name != cfgAfter.Name { + t.Fatalf("unexpected name: got %s, want %s", cfgAfter.Name, cfgBefore.Name) + } + if cfgBefore.GoRoot != cfgAfter.GoRoot { + t.Fatalf("unexpected GOROOT: got %s, want %s", cfgAfter.GoRoot, cfgBefore.GoRoot) + } + compareEnvs(t, cfgBefore.BuildEnv.Env, cfgAfter.BuildEnv.Env) + compareEnvs(t, cfgBefore.ExecEnv.Env, cfgAfter.ExecEnv.Env) + } +} + +func compareEnvs(t *testing.T, a, b *common.Env) { + t.Helper() + + aIndex := makeEnvIndex(a) + bIndex := makeEnvIndex(b) + for aKey, aVal := range aIndex { + if bVal, ok := bIndex[aKey]; !ok { + t.Errorf("%s in A but not B", aKey) + } else if aVal != bVal { + t.Errorf("%s has value %s A but %s in B", aKey, aVal, bVal) + } + } + for bKey := range bIndex { + if _, ok := aIndex[bKey]; !ok { + t.Errorf("%s in B but not A", bKey) + } + // Don't check values that exist in both. We got that already + // in the first pass. + } +} + +func makeEnvIndex(a *common.Env) map[string]string { + index := make(map[string]string) + for _, s := range a.Collapse() { + d := strings.IndexRune(s, '=') + index[s[:d]] = s[d+1:] + } + return index +} ``` ``` The name of the file: go.sum Insertions: 2, Deletions: 0. @@ -48,6 +48,8 @@ dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU= github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= +github.com/BurntSushi/toml v1.0.0 h1:dtDWrepsVPfW9H/4y7dDgFc2MBUSeJhlaDtK13CxFlU= +github.com/BurntSushi/toml v1.0.0/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ= github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= github.com/OneOfOne/xxhash v1.2.2 h1:KMrpdQIwFcEqXDklaen+P1axHaj9BSKzvpUUfnHldSE= github.com/OneOfOne/xxhash v1.2.2/go.mod h1:HSdplMjZKSmBqAxg5vPj2TmRDmfkzw+cTzAElWljhcU= ``` ``` The name of the file: go.mod Insertions: 1, Deletions: 1. @@ -4,7 +4,7 @@ require ( cloud.google.com/go/storage v1.18.2 - github.com/BurntSushi/toml v0.3.1 + github.com/BurntSushi/toml v1.0.0 github.com/biogo/biogo v1.0.4 github.com/biogo/graph v0.0.0-20150317020928-057c1989faed github.com/biogo/store v0.0.0-20201120204734-aad293a2328f ``` Patch-set: 19 Subject: sweet: add end-to-end integration test Status: merged Commit: 1bae5b3 Tag: autogenerated:gerrit:merged Groups: 1e377dd Label: Code-Review=+2, 4a5f48f680743380d4d9cc7a93da4d081ed0b02b Gerrit User 12120 <12120@62eb7196-b449-3ce5-99f1-c037f21e1705> Label: Trust=+1, ac4126a8d5cca66319d1c5004f7b3c84526527a5 Label: Run-TryBot=+1, aeb4742202a10206e08c2aed9beeb8d6f7e5dfff Label: TryBot-Result=+1, 5d8226b24fd7e545ff2cdc9a12affaac68d4a4f1 Gerrit User 5976 <5976@62eb7196-b449-3ce5-99f1-c037f21e1705> Label: SUBM=+1, 7798e335ac157b8101f32427abf66bde1570abea Submission-id: 378276 Submitted-with: OK Submitted-with: Rule-Name: gerrit~PrologRule Submitted-with: MAY: Auto-Submit Submitted-with: MAY: TryBot-Result Submitted-with: MAY: Run-TryBot Submitted-with: MAY: Trust Submitted-with: OK: Code-Review: Gerrit User 12120 <12120@62eb7196-b449-3ce5-99f1-c037f21e1705> Submitted-with: OK: Trusted: Gerrit User 5976 <5976@62eb7196-b449-3ce5-99f1-c037f21e1705> Attention: {"person_ident":"Gerrit User 12120 \u003c12120@62eb7196-b449-3ce5-99f1-c037f21e1705\u003e","operation":"REMOVE","reason":"Change was submitted"}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hi, hope you can understand this issue (if it is really an issue).
In enconde.go Line 210
If the struct implements MarshalToml to output a valid toml, this piece of code is writing the result with "double quotes" which is the same as TextMarshaler, which I don't think this is meant to this.
What I'm trying to do is to Marshall an structure to a single value, and I'm getting a quoted value
In my case if I have:
And I what to marshall this:
Without custom marshall:
With Custom Marshall:
What it actually should be
This double quotes are added outside the MarshalTOML() function and according to the interface comments, it should not be like this right?:
Or this is some expected behavior?
Is there is another way to implement what I'm trying to do and I didn't figure it out?
The text was updated successfully, but these errors were encountered: