New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move to TOML for manifest and lock #342

Merged
merged 20 commits into from Apr 13, 2017

Conversation

Projects
None yet
8 participants
@carolynvs
Copy link
Collaborator

carolynvs commented Mar 21, 2017

Fixes #119; fixes #168.

I'm doing this in stages so that we can identify when I'm off in the weeds or need to make more decisions about the config file format.

  • Vendor https://github.com/pelletier/go-toml.
  • Write manifest and lock in TOML format.
  • Read manifest and lock in TOML format (all tests but diff should pass)
  • Remove old JSON files and logic.
  • Fix output of dry-run to use TOML (diff test and all others should pass)
  • (Optional) Bike-shed the look of the generated TOML, e.g. inline tables, indenting, key sorting, etc.

@googlebot googlebot added the cla: yes label Mar 21, 2017

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Mar 21, 2017

@sdboyer @tro3 I didn't find any reflection-based marshal magic for converting between structs and TOML in this go-toml package. If we want to go with this lib, it may make sense to contribute that functionality back into the library. For the moment, I am explicitly performing the mapping in MarshalTOML.

https://github.com/golang/dep/pull/342/files#diff-8e0a04a8bff74b300db5882b4be50589R133

Do you think that makes sense or would you prefer that I try out another lib?

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Mar 21, 2017

@carolynvs hmm...that puts me on the fence about it, as I'd generally rather have the (presumably cross-TOML implementation portable) struct tags as the canonical way of expressing this information, to minimize effort in the event that we switch TOML libs later. idk how much work that's really saving, though, so it seems like a soft requirement at worst.

Maybe some input from @pelletier here would be good - any plans to add reflection-based struct tag support to go-toml? Would you accept such a change, if offered?

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Mar 21, 2017

Eh, that was too wishy-washy, sorry. If we can have struct tags and reflection-based parsing, great. If not, well, it seems like the one-off implementation here isn't too awful. Either way, not a reason to derail the current track.

Is there are reason you're keeping the JSON struct tags around?

@pelletier

This comment has been minimized.

Copy link

pelletier commented Mar 21, 2017

@sdboyer yes, reflection-based (un)marshaling is the next thing that I'd like to implement in go-toml. no eta on that though, but pull requests are welcome :)

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Mar 21, 2017

@pelletier - I have some time coming up the next few weeks. Mind if I take a crack at the reflection-based marshaling? If so, I'll open an issue at go-toml and get you a WIP PR in the short term to make sure you approve of the structure before I dive in too deep. Thoughts?

@pelletier

This comment has been minimized.

Copy link

pelletier commented Mar 21, 2017

@tro3 sounds great!

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Mar 21, 2017

@sdboyer No reason, just hadn't gotten around to removing all the json stuff yet. 😃 I've added a task to the PR to make that more clear.

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Mar 22, 2017

  • Changed manifest format. dependencies and overrides use an array of tables instead of maps now, which is easier to work with.

    ignores = ["github.com/foo/bar"]
    
    [[dependencies]]
      name = "github.com/sdboyer/gps"
      version = ">=0.12.0, <1.0.0"
    
    [[dependencies]]
      name = "github.com/babble/brook"
      revision = "d05d5aca9f895d19e9265839bffeadd74a2d2ecb"
    
    [[overrides]]
      branch = "master"
      name = "github.com/sdboyer/gps"
      source = "https://github.com/sdboyer/gps"
  • Update all the test files from JSON to TOML

  • Poor man's mapping from TOML to structs. Can't wait for pelletier/go-toml#149! ❤️

Next up I'm fixing the lock diff format and looking sorting bug (tests pass then fail due to sorting).

@omeid

This comment has been minimized.

Copy link

omeid commented Mar 23, 2017

Couldn't now overrides be just an attribute on the dependency itself?

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Mar 23, 2017

@omeid Let's not make changes to the config file structure in this PR, beyond the initial scope of switching from JSON to TOML. 😀 It's already a pretty big change, and I want to limit the amount of regressions/confusion it may cause.

@nathany

This comment has been minimized.

Copy link
Contributor

nathany commented Mar 23, 2017

For generated TOML, I think it would be nice to use inline tables to get each dependency onto a single line.

I'm think that may work better for diffs/merges, though I'm not totally sure. Also it's nice for rearranging the dependencies by movings lines up/down in an editor (for super organized types).

I can imagine counter arguments though, as lines could get pretty long when there are a number of options specified.

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Mar 23, 2017

@nathany I think that depends on how we feel about long lines in the file. Personally, I find it hard to read.

For example, here's what I think the lock would look like with inline tables. Note, I am cheating a bit by using "." for the package list, to make the shortest possible entry.

memo = ""
projects = [
 { name = "github.com/sdboyer/deptest", packages = ["."], revision = "ff2948a2ac8f538c4ecd55962e919d1e13e74baf", version = "v0.8.0" }
 { name = "github.com/sdboyer/deptestdos", packages = ["."], revision = "a0196baa11ea047dd65037287451d36b861b00ea", version = "1.0.0" }
]

Though I'm not 100% sure that's how an array of inline tables should look? Either way support for changing the formatter in go-toml would be needed, not sure about the parser.

@nathany

This comment has been minimized.

Copy link
Contributor

nathany commented Mar 23, 2017

Okay. Those are some pretty long lines.

It's probably best to just use it for a while to see how well diffs/merges work with the multi-line syntax. Thanks

@carolynvs carolynvs force-pushed the carolynvs:toml-allthethings branch from 8652142 to 5bba6bd Mar 29, 2017

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Mar 29, 2017

I've updated the lock diff to output matching TOML. I've also tried out @tro3 's reflection based mapping, it's super close and I think it's worth waiting for it, removes a TON of code. 😃

@pelletier

This comment has been minimized.

Copy link

pelletier commented Mar 29, 2017

pelletier/go-toml#149 has been merged, bringing reflection-based marshaling goodness to go-toml. props to @tro3 :)

@sdboyer sdboyer self-requested a review Mar 29, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Mar 31, 2017

We need to roll the file name change into this, too, so that we minimize the amount of change that goes in at once. We're still pondering those, but...

Cursory look has me thinking this is pretty good. I'll look more soon, I hope later tonight :)

@carolynvs carolynvs changed the title [WIP] Move to TOML for manifest and lock Move to TOML for manifest and lock Apr 1, 2017

@sdboyer
Copy link
Member

sdboyer left a comment

Mostly nits, a couple slightly bigger things.

Sorry for the slow action on this!

@@ -18,7 +18,7 @@ func TestDeriveManifestAndLock(t *testing.T) {
defer h.Cleanup()

h.TempDir("dep")
golden := "analyzer/manifest.json"
golden := "analyzer/manifest.toml"

This comment has been minimized.

@sdboyer

sdboyer Apr 1, 2017

Member

I realize you didn't introduce this, but now that I'm seeing it, I believe it should be filepath.Join("analyzer", ManifestName).

This comment has been minimized.

@tro3

tro3 Apr 1, 2017

Contributor

FWIW, being a PC guy, I've found Go does a startingly good job of using '/' in a platform-independent manner in path strings. But filepath.Join is still likely more robust.

memo = "63510efb9632ec69c1164ce396d7ebea4ad3884b4fa508373da17226d5a39739"

[[projects]]
name = "github.com/sdboyer/deptest"

This comment has been minimized.

@sdboyer

sdboyer Apr 1, 2017

Member

Hmm...yeah, I hate to bikeshed, but these indents are kinda making me twitch. What's your thinking behind indenting vs. not? Are there any typical standards out there for it? Do we know anything about, say, widespread editor defaults for indentation in TOML files?

This comment has been minimized.

@tro3

tro3 Apr 1, 2017

Contributor

The go-toml package will indent on -update. (It will read fine with or without indent.) Don't know if there is a setting to stop it - never looked.

This comment has been minimized.

@carolynvs

carolynvs Apr 2, 2017

Collaborator

Yeah, preferably go-toml would let us specify formatting options. I'll double-check that doesn't exist already and see if it's an easy thing to add.

This comment has been minimized.

@carolynvs

carolynvs Apr 2, 2017

Collaborator

Okay, it's not exposed but seems like it wouldn't be hard to add a flag to control the auto-indentation that it's currently doing. I'll submit a PR to go-toml for that.

@@ -0,0 +1,12 @@
memo = ""

This comment has been minimized.

@sdboyer

sdboyer Apr 1, 2017

Member

Any idea how this one ended up without a memo? Mostly just curious.

This comment has been minimized.

@carolynvs

carolynvs Apr 2, 2017

Collaborator

I am not sure why the memo is empty? 😄 It's been like that for a while; here's there current lock.json for that test:

https://github.com/golang/dep/blob/master/cmd/dep/testdata/harness_tests/init/case1/final/lock.json#L2

Think that's a bug?

fs.go Outdated
// Marshaler is the interface implemented by types that
// can marshal themselves into valid TOML.
// TODO(carolynvs) Consider adding this to go-toml?
type Marshaler interface {

This comment has been minimized.

@sdboyer

sdboyer Apr 1, 2017

Member

Probably better not to export this as an interface, unless we have to. As your TODO notes, if it's going to be exported, that should be done by go-toml. If we can get away with exporting, marshaler, or better tomlMarshaler would be preferable.

This comment has been minimized.

@carolynvs

carolynvs Apr 2, 2017

Collaborator

Thanks for noticing this! I had intended to put it in go-toml but that slipped my mind... 😊

This comment has been minimized.

@carolynvs

carolynvs Apr 5, 2017

Collaborator

I ended up needing this to use reflection for the LockDiff. Here's the go-toml PR: pelletier/go-toml#151

for n, pp := range rm.Dependencies {
m.Dependencies[gps.ProjectRoot(n)], err = toProps(n, pp)
for i := 0; i < len(raw.Dependencies); i++ {
name, prj, err := toProject(raw.Dependencies[i])

This comment has been minimized.

@sdboyer

sdboyer Apr 1, 2017

Member

The cost of moving to an array from a map is that it's now possible for users to express the same project more than once. This loop has to validate that each entry in the input is unique, and fail hard otherwise.

manifest.go Outdated

for n, pp := range m.Ovr {
raw.Overrides[string(n)] = toPossible(pp)
// TODO(carolynvs) when gps is moved, we can use the unexported gps.sortedConstraints

This comment has been minimized.

@sdboyer

sdboyer Apr 1, 2017

Member

It'll still be unexported within the gps package, so probably not 😄

@@ -584,7 +584,7 @@ func (h *Helper) Cleanup() {

// ReadManifest returns the manifest in the current directory.
func (h *Helper) ReadManifest() string {
m := filepath.Join(h.pwd(), "manifest.json")
m := filepath.Join(h.pwd(), "manifest.toml")

This comment has been minimized.

@sdboyer

sdboyer Apr 1, 2017

Member

Use the constant

This comment has been minimized.

@carolynvs

carolynvs Apr 2, 2017

Collaborator

I don't believe I can use the constant here (in the test pkg) because that would create a circular dependency?

This comment has been minimized.

@sdboyer

sdboyer Apr 4, 2017

Member

ahhh, totally, yes. my bad :)

This comment has been minimized.

@sdboyer

sdboyer Apr 4, 2017

Member

let's create a local constant in this test package too, and use that. it's not as good as one constant, but it's still better than string literals all over.

@@ -594,7 +594,7 @@ func (h *Helper) ReadManifest() string {

// ReadLock returns the lock in the current directory.
func (h *Helper) ReadLock() string {
l := filepath.Join(h.pwd(), "lock.json")
l := filepath.Join(h.pwd(), "lock.toml")

This comment has been minimized.

@sdboyer

sdboyer Apr 1, 2017

Member

Use the constant

}
]

Add:

This comment has been minimized.

@sdboyer

sdboyer Apr 1, 2017

Member

Hot diggity damn these look so much better 😄

Branch *StringDiff `json:"branch,omitempty"`
Revision *StringDiff `json:"revision,omitempty"`
Packages []StringDiff `json:"packages,omitempty"`
Name gps.ProjectRoot

This comment has been minimized.

@sdboyer

sdboyer Apr 1, 2017

Member

these don't need toml struct tags?

This comment has been minimized.

@carolynvs

carolynvs Apr 4, 2017

Collaborator

Sorry an emergency came up and I stopped mid review. I'll start back on this today. The current toml mapper doesn't yet support allowing a nested type to use a custom marshal function, so it was manual. I started adding that this weekend and hopefully can submit that to go-toml soon and use it.

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Apr 5, 2017

@sdboyer You mentioned rolling up the filename changes into this PR to limit churn. Do we have final names yet?

carolynvs added some commits Mar 20, 2017

Vendor pelletier/go-toml TOM library and deps
github.com/pelletier/go-toml@fee7787d3f811af92276f5ff10107092e95b7a1d
 - Can't use latest release, it has a show-stopping bug in ToTomlString
github.com/pelletier/go-buffruneio@v0.2.0
Represent manifest dependencies as an array of tables, not a map
Using the project root as a key in TOML makes our life harder. This
siplifies the manifest format and parsing.
@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Apr 5, 2017

@sdboyer @tro3 Right now go-toml doesn't preserve the ordering of the fields when marshaling, instead simply outputting them in alphabetical order:

[[projects]]
  branch = "master"
  name = "github.com/pelletier/go-buffruneio"
  packages = ["."]
  revision = "c37440a7cf42ac63b919c752ca73a85067e05992"

instead of this (which matches the ordering of the fields as defined on the struct which is what encoding/json does)

[[projects]]
  name = "github.com/pelletier/go-buffruneio"
  branch = "master"
  revision = "c37440a7cf42ac63b919c752ca73a85067e05992"
  packages = ["."]

Is this something that I need to address as part of this PR, in the name of not changing the file structure again later? It may be a big change to how the TomlTree is represented internally and could be a lot of work, so I wanted to get your thoughts (or maybe @tro3 you see a clean way of supporting that)?

@carolynvs carolynvs force-pushed the carolynvs:toml-allthethings branch from 079a526 to 920e5d4 Apr 5, 2017

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Apr 5, 2017

  • Rebased, resolving the merge conflicts from updating gps
  • Added constants for the file names to the lock package
  • Updated LockDiff formatting to the toml struct tags

Looking into making the auto-indentation configurable next.

carolynvs added some commits Apr 5, 2017

Use a constant for the manifest/lock file names
We can’t reference dep.ManifestName/LockName because it would create a circular reference

@carolynvs carolynvs force-pushed the carolynvs:toml-allthethings branch from 920e5d4 to a068a3c Apr 5, 2017

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Apr 5, 2017

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Apr 6, 2017

I spoke with @sdboyer and we will leave off trying to make the toml file format "pretty", so I am not going to try to tackle the indenting or sorting in this PR. I believe I've hit all the rest of the review feedback so it's ready for another look.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 6, 2017

OK for filenames, let's do Gopkg.toml and Gopkg.lock.

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Apr 6, 2017

File renaming is in! 💥

@bradleyfalzon

This comment has been minimized.

Copy link
Contributor

bradleyfalzon commented Apr 8, 2017

Is it possible, or was it already decided not, to read the old format and update the relevant files?

As the migration path, as I understand it, would be to remove the old files and generate new lock files - which would lock to different revisions?

Detecting existing files, reading them and rewriting to new files and new format would obviously be ideal from an early user's perspective. Perhaps a flag, or a separate application (depfix)?

I don't see this as a major requirement at all, just asking.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 13, 2017

Sorry for delay on merge, here. I was trying to get things together with a blog for dep (re: #294), and I put this in the queue behind doing that. Looks like I should have that blog done tonight, and I plan to merge this once it's all set up 😄

@sdboyer sdboyer merged commit b839831 into golang:master Apr 13, 2017

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 13, 2017

And in, finally. Thanks! 🎉 🎉 🎉 🎉

elfgoh added a commit to elfgoh/vuls that referenced this pull request Apr 13, 2017

@elfgoh elfgoh referenced this pull request Apr 13, 2017

Merged

Fixing #420 where lock and manifest have moved to TOML #421

4 of 8 tasks complete

@smira smira referenced this pull request Apr 13, 2017

Closed

Update `dep` lock files #549

kotakanbe added a commit to future-architect/vuls that referenced this pull request Apr 14, 2017

ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017

@carolynvs carolynvs deleted the carolynvs:toml-allthethings branch Jun 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment