search

basecamp / thruster

MIT License
672 stars 16 forks source link

Kevin's local build directory being shown? #21

Closed exegeteio closed 2 months ago

exegeteio commented 2 months ago

It appears that the folder on @kevinmcconnell 's machine where the app was built is being output when the go app errors out:


$ bundle exec thrust ./app.rb
{"time":"2024-04-28T12:25:33.26878-07:00","level":"INFO","msg":"Server started","http":":80"}
{"time":"2024-04-28T12:25:33.269463-07:00","level":"INFO","msg":"Server stopping"}
{"time":"2024-04-28T12:25:33.26952-07:00","level":"INFO","msg":"Server stopped"}
panic: fork/exec ./app.rb: permission denied

goroutine 1 [running]:
github.com/basecamp/thruster/internal.(*Service).Run(0x1400003ff20)
        /Users/kevin/Work/basecamp/thruster/internal/service.go:40 +0x2a4
main.main()                                                                                                                     /Users/kevin/Work/basecamp/thruster/cmd/thrust/main.go:25 +0x94```

In my case, `app.rb` is not executable and I should have used `thrust ruby ./app.rb`.  I did find it rather worrying to see Kevin's home directory when I trued to run the command, though.

Not swish enough with Go to suggest a fix here.  But if anyone has suggestions of what to Google for, I'd love to contribute a PR.
kevinmcconnell commented 2 months ago

Hi @exegeteio, it’s default Go behaviour to include the source paths in stack traces during a panic. I don’t think it’s really a problem in this case, but I agree it can be surprising.

I’ll update the makefile to trim those paths out of the release builds (there’s a compiler flag to do that). Thanks for bringing this up!

kevinmcconnell commented 2 months ago

But if anyone has suggestions of what to Google for, I'd love to contribute a PR.

Oops sorry, missed that part on first read! If you fancy taking a stab at this, take a look at the -trimpath flag for go build. I think we’ll want to add that in the Makefile. The dist target is what’s used to make release builds.

kevinmcconnell commented 2 months ago

@exegeteio I've also added #22, for the related issue that we shouldn't be showing a stack trace in this situation anyway (regardless of what's in that stack trace).