rocketlaunchr / dataframe-go

DataFrames for Go: For statistics, machine-learning, and data manipulation/exploration
Other
1.19k stars 95 forks source link

Potential collision and risk from indirect dependence "github.com/gotestyourself/gotestyourself" #37

Closed KateGo520 closed 4 years ago

KateGo520 commented 4 years ago

Background

Repo rocketlaunchr/dataframe-go used the old path to import gotestyourself indirectly. This caused that github.com/gotestyourself/gotestyourself and gotest.tools coexist in this repo: https://github.com/rocketlaunchr/dataframe-go/blob/master/go.mod (Line 20 & 40)

github.com/gotestyourself/gotestyourself v2.2.0+incompatible // indirect
gotest.tools v2.2.0+incompatible // indirect 

That’s because the gotestyourself has already renamed it’s import path from "github.com/gotestyourself/gotestyourself" to "gotest.tools". When you use the old path "github.com/gotestyourself/gotestyourself" to import the gotestyourself, will reintroduces gotestyourself through the import statements "import gotest.tools" in the go source file of gotestyourself.

https://github.com/gotestyourself/gotest.tools/blob/v2.2.0/fs/example_test.go#L8

package fs_test
import (
    …
    "gotest.tools/assert"
    "gotest.tools/assert/cmp"
    "gotest.tools/fs"
    "gotest.tools/golden"
)

"github.com/gotestyourself/gotestyourself" and "gotest.tools" are the same repos. This will work in isolation, bring about potential risks and problems.

Solution

Add replace statement in the go.mod file:

replace github.com/gotestyourself/gotestyourself => gotest.tools v2.2.0

Then clean the go.mod.

KateGo520 commented 4 years ago

@rocketlaunchr-cto @propersam Could you help me review this issue? Thx :p

pjebs commented 4 years ago

I'll have a look at it within 2 days. Should be an easy fix.

pjebs commented 4 years ago

done

KateGo520 commented 4 years ago

@pjebs Thank you for your contribution.

pjebs commented 4 years ago

@KateGo520 I may have to revert this because it looks like go test doesn't work with the replace directive.

pjebs commented 4 years ago

I think your only concern is deduplication of essentially the same package (increase file size), which is not a "collision risk" in Go.

By the looks of it, you may need to file a bug report with regards to go test so it accepts the replace directive in this scenario, or lobby all packages to correct their path dependency to the new path and/or (perhaps in some cases) use the Go alias declaration feature.