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

sync: update CI config files #20

Closed
wants to merge 9 commits into from
Closed

sync: update CI config files #20

wants to merge 9 commits into from

Conversation

web3-bot
Copy link
Collaborator

@web3-bot web3-bot commented Dec 10, 2021

Syncing to commit protocol/.github@2821662.

@galargh
Copy link

galargh commented Dec 13, 2021

@mvdan it looks like here the addition of -coverpkg=./... results in the following error:

panic: /tmp/go-build479579212/b001/zeroconf.test flag redefined: name

Do you know why that might be? I've been able to reproduce this locally as well.

@mvdan
Copy link

mvdan commented Dec 19, 2021

Here are the flags defined twice, but not really:

$ git grep '"name"'
examples/proxyservice/server.go:    name     = flag.String("name", "GoZeroconfGo", "The name for the service.")
examples/register/server.go:    name     = flag.String("name", "GoZeroconfGo", "The name for the service.")

This is not really a bug in our code, nor our fault, but it's still what is triggering the error. The umbrella issue upstream is golang/go#23910.

The short version is that, to capture coverage for all packages with -coverpkg=./..., go test forces each package's test binary to import and link all packages in ./.... Building both of those example packages into the same main package means that both init funcs try to register the name flag, hence the error.

The right fix is to fix that bug upstream, because it shouldn't be adding imports which aren't necessary.

I had forgotten about that bug, though. Without a fix, -coverpkg=./... is just a bit unreliable.

I'll take a closer look at this when I have more time. I think we might be able to use a bit of scripting to work around the bug.

@mvdan mvdan mentioned this pull request Dec 20, 2021
@mvdan
Copy link

mvdan commented Dec 20, 2021

How about this monstrosity?

go test -coverpkg=$(go list -json ./... | jq -rs '[.[] | select(.Name != "main") | .ImportPath] | join(",")') ./...

Essentially, -coverpkg=./... makes all packages count towards each package being tested. The problem arises with main packages, whose init funcs may collide with declared flags or other global state that may need to be unique.

The go list and jq command above filters the main packages out of ./..., and prints the output in the format that -coverpkg wants - a comma-separated list. It should work.

There's one major caveat, though - if we're testing a main package, then it won't count towards its own coverage. -coverpkg replaces the default of "current package being tested", it's not in addition to the current package. So it feels like a step backwards from not supplying -coverpkg at all.

I think we should call it quits with -coverpkg. When I added the flag, I forgot about this bug with main packages. I do think the upstream bug will be fixed, but it may be one or two releases until that happens. cc @marten-seemann

@marten-seemann
Copy link

I think we should call it quits with -coverpkg. When I added the flag, I forgot about this bug with main packages. I do think the upstream bug will be fixed, but it may be one or two releases until that happens. cc @marten-seemann

I'm fine with dropping this, but let's not do another deployment run for this. It looks like zeroconf is the only package that's affected, and we can live with the slightly outdated CI config for another 2 months.

@galargh
Copy link

galargh commented Dec 21, 2021

I'm fine with dropping this, but let's not do another deployment run for this. It looks like zeroconf is the only package that's affected, and we can live with the slightly outdated CI config for another 2 months.

I can confirm that I haven't seen a similar looking error in any other repository.

@web3-bot web3-bot force-pushed the web3-bot/sync branch 6 times, most recently from be8d76b to 9f218ce Compare April 11, 2022 07:18
@web3-bot web3-bot force-pushed the web3-bot/sync branch 2 times, most recently from f8248a0 to a7bd0f7 Compare May 10, 2022 10:15
@web3-bot web3-bot force-pushed the web3-bot/sync branch 3 times, most recently from 5e948bd to 1bdf660 Compare June 7, 2022 21:42
@web3-bot web3-bot force-pushed the web3-bot/sync branch 2 times, most recently from 252e273 to 18fc83e Compare June 16, 2022 12:37
@web3-bot web3-bot force-pushed the web3-bot/sync branch 2 times, most recently from 10e4511 to 82e227a Compare June 29, 2022 19:38
@web3-bot web3-bot force-pushed the web3-bot/sync branch 3 times, most recently from 12b99d2 to 7d8b523 Compare July 11, 2022 18:31
@web3-bot web3-bot force-pushed the web3-bot/sync branch 7 times, most recently from 8737106 to 143593a Compare February 3, 2023 11:21
@web3-bot web3-bot force-pushed the web3-bot/sync branch 5 times, most recently from 8207223 to 4b745e6 Compare February 8, 2023 12:58
@web3-bot web3-bot force-pushed the web3-bot/sync branch 3 times, most recently from 85e52dc to a7ffb72 Compare March 2, 2023 10:42
@web3-bot web3-bot force-pushed the web3-bot/sync branch 4 times, most recently from 3c16c90 to 0ceaf84 Compare March 15, 2023 11:24
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 this pull request may close these issues.

4 participants