kynrai / tainted

Tool to determine which Go packages need to be rebuilt in a monorepo
MIT License
58 stars 5 forks source link

Root package changes not being considered for tainting #2

Open TomDeVito opened 6 years ago

TomDeVito commented 6 years ago

Hey, thanks for making this tool, I ran into a little snag:

If I have the following structure:

cmd/
    app1/
        main.go     <- imports package project
    app2/
         main.go
project.go          <-  package name:  project

and I make changes to project.go, app1 and app2 are not considered to be tainted. project.go is in the CWD, but seems to be ignored here

When I change the code to the following:

if dir := filepath.Dir(scanner.Text()); dir != "." {
    changedDirs[dir] = struct{}{}
} else {
    ctx := build.Default
    cwd, err := os.Getwd()
    if err != nil {
        log.Fatal(err)
    }
    p, err := ctx.ImportDir(cwd, build.FindOnly)
    if err != nil {
        log.Fatal(err)
    }
    changedDirs[p.ImportPath] = struct{}{}
}

I seem to get my desired outcome. I'm not sure if this the best way to handle this or not and was wondering what your thoughts are about this matter.

Thanks,

kynrai commented 6 years ago

Hello @TomDeVito ,

Thanks for filing this. I must admit i never considered this case as .go files are often not in the root when following the recommended go structure, however I can totally see many situations where it would be the case.

Ill run some tests but I suspect removing the explicit check for root packages would fix this, I think I originally put the check for packages in . as an optimisation or that it was causing all packages to be marked as tainted.

Ill have a play.

kynrai commented 6 years ago

Also apologies for the late reply @TomDeVito

TomDeVito commented 6 years ago

No worries, thanks for responding. I'm following Ben Johnson's Standard Package Layout which has domain structs at the root level. Yea, I tried removing it, but didn't find it working properly until I was able to discover what package was contained in the cwd

tbruyelle commented 6 years ago

I had the same issue and @TomDeVito's patch fixed it.