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

code generators are silent #53791

Closed
antoineco opened this Issue Oct 12, 2017 · 9 comments

Comments

Projects
None yet
7 participants
@antoineco
Contributor

antoineco commented Oct 12, 2017

What happened:

Invoking deepcopy-gen (and other code generators) does not output any information regardless of the selected log level, making the generation flow harder to troubleshoot.

What you expected to happen:

deepcopy-gen is verbose about what it does when the loglevel is set.

How to reproduce it (as minimally and precisely as possible):

Navigate to k8s.io/apiextensions-apiserver/examples/client-go and execute:

$ deepcopy-gen -v 5 -i ./apis/cr/v1 -h /dev/null
(no output)

Anything else we need to know?:

Using an updated glog package solves the problem, which seems surprising to me looking at the commit history.

$ deepcopy-gen -v 5 -i ./apis/cr/v1 -h /dev/null
...
ERROR: logging before flag.Parse: I1012 13:05:22.300800   39771 deepcopy.go:545] Generating deepcopy function for type k8s.io/apiextensions-apiserver/examples/client-go/apis/cr/v1.ExampleSpec
ERROR: logging before flag.Parse: I1012 13:05:22.301055   39771 deepcopy.go:545] Generating deepcopy function for type k8s.io/apiextensions-apiserver/examples/client-go/apis/cr/v1.ExampleStatus
ERROR: logging before flag.Parse: I1012 13:05:22.301313   39771 execute.go:67] Assembling file "/go/src/k8s.io/apiextensions-apiserver/examples/client-go/apis/cr/v1/deepcopy_generated.go"
ERROR: logging before flag.Parse: I1012 13:05:22.307813   39771 main.go:87] Completed successfully.
@mml

This comment has been minimized.

Show comment
Hide comment
@mml

mml Oct 12, 2017

Contributor

@antoineco if you have a PR to fix this, we will happily take it.

Contributor

mml commented Oct 12, 2017

@antoineco if you have a PR to fix this, we will happily take it.

@antoineco

This comment has been minimized.

Show comment
Hide comment
@antoineco

antoineco Oct 14, 2017

Contributor

The short answer seems to be that pflag ignores the standard go flagset. However this is supposed to be covered by this snippet, and from my experience that works fine:

if g.defaultCommandLineFlags {
g.AddFlags(pflag.CommandLine)
pflag.CommandLine.AddGoFlagSet(goflag.CommandLine)
pflag.Parse()
}

I can't figure out why the loglevel is not set properly in that particular case. @sttts any idea maybe?

Contributor

antoineco commented Oct 14, 2017

The short answer seems to be that pflag ignores the standard go flagset. However this is supposed to be covered by this snippet, and from my experience that works fine:

if g.defaultCommandLineFlags {
g.AddFlags(pflag.CommandLine)
pflag.CommandLine.AddGoFlagSet(goflag.CommandLine)
pflag.Parse()
}

I can't figure out why the loglevel is not set properly in that particular case. @sttts any idea maybe?

@dims

This comment has been minimized.

Show comment
Hide comment
@dims

dims Oct 16, 2017

Member

@antoineco - did the --alsologtostderr and --logtostderr not help you?

Member

dims commented Oct 16, 2017

@antoineco - did the --alsologtostderr and --logtostderr not help you?

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Oct 16, 2017

Contributor

Have to take a look. I also saw a case that client-gen did not accept standard go flags.

Contributor

sttts commented Oct 16, 2017

Have to take a look. I also saw a case that client-gen did not accept standard go flags.

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Oct 17, 2017

Contributor

Which branch of k8s.io/code-generator is this about? master or release-1.8?

Contributor

sttts commented Oct 17, 2017

Which branch of k8s.io/code-generator is this about? master or release-1.8?

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Oct 17, 2017

Contributor

deepcopy-gen -v 5 -i ./apis/cr/v1 -h /dev/null --alsologtostderr gives output. Isn't that the normal behaviour of glog that nothing is printed? Actually, I wouldn't mind to switch to --alsologtostderr by default though.

Contributor

sttts commented Oct 17, 2017

deepcopy-gen -v 5 -i ./apis/cr/v1 -h /dev/null --alsologtostderr gives output. Isn't that the normal behaviour of glog that nothing is printed? Actually, I wouldn't mind to switch to --alsologtostderr by default though.

@antoineco

This comment has been minimized.

Show comment
Hide comment
@antoineco

antoineco Oct 17, 2017

Contributor

@stts I'm on the master branch. I haven't tried --alsologtostderr, I was expecting everything to be written either on stdout or stderr without using any extra flag.

Like I said, if I delete the vendored glog and use master logs are written simply by setting the V flag. Probably a misunderstanding.

(and just to provide a little bit more context, the issue I was trying to troubleshoot was that my deepcopy file didn't get generated. I figured out the deepcopy flag was not in doc.go but in another file, but without output this took me a while to figure. Newbie experience report ftw :)

Contributor

antoineco commented Oct 17, 2017

@stts I'm on the master branch. I haven't tried --alsologtostderr, I was expecting everything to be written either on stdout or stderr without using any extra flag.

Like I said, if I delete the vendored glog and use master logs are written simply by setting the V flag. Probably a misunderstanding.

(and just to provide a little bit more context, the issue I was trying to troubleshoot was that my deepcopy file didn't get generated. I figured out the deepcopy flag was not in doc.go but in another file, but without output this took me a while to figure. Newbie experience report ftw :)

@antoineco

This comment has been minimized.

Show comment
Hide comment
@antoineco

antoineco Nov 27, 2017

Contributor

@sttts @dims sorry for taking so long to investigate.

Now that I got more familiar with glog I understand what you meant with --alsologtostderr and --logtostderr: the level of verbosity doesn't matter because glog writes everything to temp files and nothing to stdout/err by default.

I figured most projects I've been using set flag.Set("logtostderr", "true") explicitly, which makes a lot of sense for CLI tools like these code generators in my opinion.

Would you be open to a PR that sets this as the default for all generators?

Contributor

antoineco commented Nov 27, 2017

@sttts @dims sorry for taking so long to investigate.

Now that I got more familiar with glog I understand what you meant with --alsologtostderr and --logtostderr: the level of verbosity doesn't matter because glog writes everything to temp files and nothing to stdout/err by default.

I figured most projects I've been using set flag.Set("logtostderr", "true") explicitly, which makes a lot of sense for CLI tools like these code generators in my opinion.

Would you be open to a PR that sets this as the default for all generators?

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Nov 27, 2017

Contributor

Would you be open to a PR that sets this as the default for all generators?

Yes, would be great. Please use #56403 as the base.

Contributor

sttts commented Nov 27, 2017

Would you be open to a PR that sets this as the default for all generators?

Yes, would be great. Please use #56403 as the base.

@antoineco antoineco changed the title from deepcopy-gen is silent to code generators are silent Jan 9, 2018

k8s-merge-robot added a commit that referenced this issue Jan 10, 2018

Merge pull request #58003 from antoineco/gen-cli-logtostderr
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Make code generators log to stderr by default

**What this PR does / why we need it**:

Most code generators inside `staging/` are CLI tools. It makes sense for CLI tools (in general) to log to stdout/err, especially knowing that [`glog` has a 30sec flush interval](https://github.com/kubernetes/kubernetes/blob/a227c1ea2cb5e1bfee3a34362c225784f0bb4af7/pkg/kubectl/util/logs/logs.go#L49), which leads to logs being lost and makes troubleshooting tedious for people not aware of that quirk.

Fixes #53791

```release-note
NONE
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment