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

PkgPath contains vendor path prefix #916

Closed
rsteube opened this issue Oct 21, 2020 · 9 comments · Fixed by #944
Closed

PkgPath contains vendor path prefix #916

rsteube opened this issue Oct 21, 2020 · 9 comments · Fixed by #944
Milestone

Comments

@rsteube
Copy link
Contributor

rsteube commented Oct 21, 2020

PkgPath can contain the vendor path prefix when using vendored dependencies in a traefik plugin.

equals then tries to compare an id with the prefix with one without it:

"*github.com/rsteube/traefik-plugin-brotli/vendor/github.com/andybalholm/brotli.encoderDictionary" == "`*github.com/andybalholm/brotli.encoderDictionary"

The following triggers an unexpected error:

# hash.go
type hasherCommon struct {
	params           hasherParams
	is_prepared_     bool
	dict_num_lookups uint
	dict_num_matches uint
}

type hasherHandle interface {
	Common() *hasherCommon
	Initialize(params *encoderParams)
	Prepare(one_shot bool, input_size uint, data []byte)
	StitchToPreviousBlock(num_bytes uint, position uint, ringbuffer []byte, ringbuffer_mask uint)
	HashTypeLength() uint
	StoreLookahead() uint
	PrepareDistanceCache(distance_cache []int)
	FindLongestMatch(dictionary *encoderDictionary, data []byte, ring_buffer_mask uint, distance_cache []int, cur_ix uint, max_length uint, max_backward uint, gap uint, max_distance uint, out *hasherSearchResult)
	StoreRange(data []byte, mask uint, ix_start uint, ix_end uint)
	Store(data []byte, mask uint, ix uint)
}

# h10.go
type h10 struct {
	hasherCommon
	window_mask_ uint
	buckets_     [1 << 17]uint32
	invalid_pos_ uint32
	forest       []uint32
}

func (*h10) FindLongestMatch(dictionary *encoderDictionary, data []byte, ring_buffer_mask uint, distance_cache []int, cur_ix uint, max_length uint, max_backward uint, gap uint, max_distance uint, out *hasherSearchResult) {
	panic("unimplemented")
}


# encode.go -  where the error occurs
createZopfliBackwardReferences(uint(bytes), uint(wrapped_last_processed_pos), data, uint(mask), &s.params, s.hasher_.(*h10), s.dist_cache_[:], &s.last_insert_len_, &s.commands, &s.num_literals_)

Expected result:

executes without error

Got:

/plugins/go/src/github.com/rsteube/traefik-plugin-brotli/vendor/github.com/andybalholm/brotli/encode.go:812:110:
impossible type assertion: *github.com/andybalholm/brotli.h10
does not implement github.com/rsteube/traefik-plugin-brotli/vendor/github.com/andybalholm/brotli.hasherHandle

To fix this the prefix might be removed from PkgPath as others have done.

see also golang/go#12739

@mvertes
Copy link
Member

mvertes commented Oct 22, 2020

Why is this causing problem ? Do you have a specific example, failing without, and succeeding with the behavior you propose ?

@rsteube
Copy link
Contributor Author

rsteube commented Oct 22, 2020

Strill trying to recreate it. It fails with brotli at:

/plugins/go/src/github.com/rsteube/traefik-plugin-brotli/vendor/github.com/andybalholm/brotli/encode.go:812:110:
impossible type assertion: *github.com/andybalholm/brotli.h10
does not implement github.com/rsteube/traefik-plugin-brotli/vendor/github.com/andybalholm/brotli.hasherHandle

When removing the prefix i get to to a different error: hash_forgetful_chain.go:77:39: CFG post-order panic: reflect: call of reflect.Value.Interface on zero Value, so i currently assume that it fixes the error (which might be wrong of course as the brotli code is not yet completely working).

@rsteube
Copy link
Contributor Author

rsteube commented Oct 22, 2020

Updated the example with snippets from brotli related to the error, this is incomplete though and no executable sample.go yet.

@rsteube
Copy link
Contributor Author

rsteube commented Oct 22, 2020

would need to be applied to t.path and t.rtype.PkgPath()

// id returns a unique type identificator string.
func (t *itype) id() (res string) {
    if t.name != "" {
        if t.path != "" {
            return t.path + "." + t.name
            return fixPkgPathVendoring(t.path) + "." + t.name
        }
        return t.name
    }
@ interp/type.go:1121 @ func (t *itype) id() (res string) {
    case valueT:
        res = ""
        if t.rtype.PkgPath() != "" {
            res += t.rtype.PkgPath() + "."
            res += fixPkgPathVendoring(t.rtype.PkgPath()) + "."
        }
        res += t.rtype.Name()
    }

@rsteube
Copy link
Contributor Author

rsteube commented Oct 30, 2020

keeping a branch open where the error occurs:
https://github.com/rsteube/traefik-plugin-brotli/tree/vendor-prefix-bug

docker-compose up

@mvertes
Copy link
Member

mvertes commented Nov 5, 2020

Hello @rsteube, could you check if #944 improves this particular issue as well ?
Thanks!

@rsteube
Copy link
Contributor Author

rsteube commented Nov 5, 2020

will do

@rsteube
Copy link
Contributor Author

rsteube commented Nov 5, 2020

@mvertes yes, boots up fine and plugin is visible in dashboard.
Invocation of brotli itself fails the same way as with my "workaround" due to a different error so i would say it fixes the issue.

@mvertes
Copy link
Member

mvertes commented Nov 5, 2020

ok thanks. Other changes are coming to fix the remaining issues in brotli.

traefiker pushed a commit that referenced this issue Nov 5, 2020
When running GTA, the type `path` was set to `rpath`. This equates to the package path (`github.com/traefik/yaegi`) in most cases. In the vendored case the `rpath` is the sub package path `something/vendor/github.com/traefik/yaegi` causing issues in typecheck and likely further down the line. By using the `importPath` it makes this consistent.

**Note:** I have no clue how to test this decently. I am open to options here.

Potentially Fixes #916
@traefiker traefiker added this to the v0.9.x milestone Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants