Skip to content
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

Update ugorji/go to latest #5676

Merged
merged 1 commit into from
Aug 23, 2019
Merged

Update ugorji/go to latest #5676

merged 1 commit into from
Aug 23, 2019

Conversation

notnoop
Copy link
Contributor

@notnoop notnoop commented May 9, 2019

fixes #4454

Our testing so far indicates that ugorji/go/codec maintains backward
compatiblity with the version we are using now, for purposes of Nomad
serialization.

Using latest ugorji/go allows us to get back to using upstream library,
get get the optimizations benefits in RPC paths (including code
generation optimizations).

ugorji/go introduced two significant changes:

  • time binary format in ugorji/go@debb8e2. Setting h.BasicHandle.TimeNotBuiltin = true restores old behavior
  • ugorji/go started honoring json tag as well:

v1.1.4 is the latest but has a bug in handling RawString that's fixed in
ugorji/go@d09a80c
.

Our testing so far indicates that ugorji/go/codec maintains backward
compatiblity with the version we are using now, for purposes of Nomad
serialization.

Using latest ugorji/go allows us to get back to using upstream library,
get get the optimizations benefits in RPC paths (including code
generation optimizations).

ugorji/go introduced two significant changes:
* time binary format in ugorji/go@debb8e2.  Setting `h.BasicHandle.TimeNotBuiltin = true` restores old behavior
* ugorji/go started honoring `json` tag as well:

v1.1.4 is the latest but has a bug in handling RawString that's fixed in
ugorji/go@d09a80c
.
@towe75
Copy link
Contributor

towe75 commented Jun 28, 2019

I worked on #5312 and noticed a dependency problem because both libpod and nomad depend on ugorji/go
So i tried to build against this branch but could not manage to get a working environment.

Here is a simple test using go 1.12 and a go module:

in go.mod

module github.com/pascomnet/justatest

go 1.12

require (
	github.com/hashicorp/nomad v0.9.1-0.20190509233558-a8a6515e9f73
	github.com/ugorji/go v1.1.5-pre 
)

in main.go

package main

import (
	"github.com/hashicorp/nomad/plugins"
)

func main() {
	d := plugins.PluginDevice
	if (d.DoneContext.Err() == nil) {
		// just a test
	}
}

go build says then:

# github.com/hashicorp/nomad/nomad/structs
/root/go/pkg/mod/github.com/hashicorp/[email protected]/nomad/structs/structs.go:8778:15: h.BasicHandle.TimeNotBuiltin undefined (type "github.com/hashicorp/go-msgpack/codec".BasicHandle has no field or method TimeNotBuiltin)

Building against nomad v0.9.2 (release) yields much more errors, though.
Can you please check or clearify if i am doing something wrong here?

@notnoop
Copy link
Contributor Author

notnoop commented Aug 23, 2019

I've tested with latest ugorji/go version (v.1.1.7) and discovered that it handles nil in map values differently, where it deserializes a nil reference as a reference of zero value. The regression seems to be due to ugorji/go@ffda9d4 .

Merging this as-is for this to soak in while we run testing and do a write up and follow up with ugorji/go library.

@notnoop notnoop merged commit a72a0f8 into master Aug 23, 2019
@notnoop notnoop deleted the f-b-upgrade-ugorji-dep-20190508 branch August 23, 2019 22:29
@notnoop
Copy link
Contributor Author

notnoop commented Aug 23, 2019

@towe75, I'm afraid I don't have much experience with go mod. The dependency on ugorji/go seems very brittle right now. I'd suggest testing with using the exact version of ugorji/go as used by nomad and specified in vendor/vendor.json file for the version you depend on.

@onlyjob
Copy link
Contributor

onlyjob commented Oct 23, 2019

"latest" should only ever mean formal, tagged semantic release: https://github.com/ugorji/go/releases

It is a bad practice to vendor by some random commit in-between releases.

Please vendor properly.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Still uses "go-msgpack/codec" - unmaintained fork of "ugorji/go/codec"
4 participants