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

version: don't panic if read build info doesn't work #5210

Merged
merged 2 commits into from
Dec 19, 2022

Conversation

lukedirtwalker
Copy link
Contributor

If debug.ReadBuildInfo() doesn't return the build information we should not try to access it. Especially if users only want to build with the CustomVersion we should not assume access to debug.ReadBuildInfo().

The build environment where this isn't available for me is when building with bazel.

@mohammed90
Copy link
Member

If debug.ReadBuildInfo() doesn't return the build information we should not try to access it. Especially if users only want to build with the CustomVersion we should not assume access to debug.ReadBuildInfo().

debug.ReadBuildInfo() returns bi, ok, where ok enables the only situation to set module (*debug.Module). I wonder if it's possible to invert the condition, and exit early if !ok. Do you think this is possible?

@lukedirtwalker
Copy link
Contributor Author

It's probably possible, those nested ifs might make it hard to get it right though. I can give it a try.

@lukedirtwalker
Copy link
Contributor Author

@mohammed90 I added a new commit with an early exit branch. Overall I don't think we can save much more ifs, without losing all of the functionality. Let me know which version you think is better.

@mholt
Copy link
Member

mholt commented Nov 18, 2022

@lukedirtwalker Thanks for the PR! And thanks for the review @mohammed90 .

Are there any differences in the outputs of these common build methods? i.e. I'm wondering if this patch changes anything, or if it simply avoids the panic. (I'd prefer to just avoid the panic.)

  • go build
  • go run
  • xcaddy build
  • bazel

@mholt mholt added the under review 🧐 Review is pending before merging label Nov 18, 2022
If `debug.ReadBuildInfo()` doesn't return the build information we
should not try to access it. Especially if users only want to build with
the `CustomVersion` we should not assume access to
`debug.ReadBuildInfo()`.

The build environment where this isn't available for me is when building
with bazel.
@lukedirtwalker
Copy link
Contributor Author

Finally had time to check it:

Currently (e10ed7b):

  • go build

    go build ./cmd/caddy/ && ./caddy version
    e10ed7b00d30cc91662949671f0051cd16da9222+modified (12 Dec 22 17:18 UTC)
    
  • go run

    go run ./cmd/caddy/ version
    unknown
    
  • xcaddy build (xcaddy build --with github.com/caddyserver/caddy/v2=/home/luke/Programming/caddy/)

    ./caddy version
    v2.6.2 => /home/luke/Programming/caddy/@(devel)
    
  • bazel (bazel run is essentially bazel build followed by an execution of the binary.)

    bazel run //cmd/caddy -- version
    panic: runtime error: invalid memory address or nil pointer dereference
    [signal SIGSEGV: segmentation violation code=0x1 addr=0x78 pc=0xab650f]
    
    goroutine 1 [running]:
    github.com/caddyserver/caddy/v2.Version()
            external/com_github_caddyserver_caddy_v2/caddy.go:897 +0x22f
    github.com/caddyserver/caddy/v2/cmd.init.2()
            external/com_github_caddyserver_caddy_v2/cmd/main.go:42 +0x1d
    

With patch based on e10ed7b:

  • go build

    go build ./cmd/caddy/ && ./caddy version
    5675fd2df9139f3892ce5cc287f9e75de252e8e3+modified (18 Dec 22 09:42 UTC)
    
  • go run

    go run ./cmd/caddy/ version
    unknown
    
  • xcaddy build (xcaddy build --with github.com/caddyserver/caddy/v2=/home/luke/Programming/caddy/)

    ./caddy version
    v2.6.2 => /home/luke/Programming/caddy/@(devel)
    
  • bazel (using x_defs to set CustomVersion variable)

    bazel run //cmd/caddy -- version
    2.6.2~anapaya2
    

    Without x_defs for CustomVersion the output is unknown

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, so that looks like there are no changes to the current output, and simply fixes the panic. Thank you!

@mholt mholt merged commit c3b5b18 into caddyserver:master Dec 19, 2022
@mholt mholt removed the under review 🧐 Review is pending before merging label Dec 19, 2022
@mholt mholt added this to the v2.6.3 milestone Dec 19, 2022
mholt pushed a commit that referenced this pull request Feb 6, 2023
* version: don't panic if read build info doesn't work

If `debug.ReadBuildInfo()` doesn't return the build information we
should not try to access it. Especially if users only want to build with
the `CustomVersion` we should not assume access to
`debug.ReadBuildInfo()`.

The build environment where this isn't available for me is when building
with bazel.

* exit early
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.

3 participants