Add debug flag to include delve #1320
Conversation
There was a problem hiding this comment.
This looks good, thank you @creydr especially for your work and patience, and for putting up with us.
In addition to the CLI docs it'd be great to have some usage docs for the website, but that can happen later, when this is released.
pkg/build/gobuild.go
Outdated
|
|
||
| // find the delve binary in tmpInstallDir/bin/ | ||
| delveBinary := "" | ||
| err = filepath.WalkDir(filepath.Join(tmpInstallDir, "bin"), func(path string, d fs.DirEntry, err error) error { |
There was a problem hiding this comment.
Why do we have to walk? Don't we know it'll be at tmpInstallDir/bin/dlv?
There was a problem hiding this comment.
That's a good point... We probably want to confirm the end state after the "go install", but IIUC we could do that more directly with a stat call with the expected path.
There was a problem hiding this comment.
Ah I just hit a snag here that I'm guessing motivated this design: testing again locally, the binary was at tmpInstallDir/bin/linux_arm64/dlv.
There was a problem hiding this comment.
Hey @imjasonh,
the motivation behind this is as @luhring said in #1320 (comment): for other archs it can be located at different locations/folders.
There was a problem hiding this comment.
That makes sense! In that case, should we only look at tmpInstallDir/bin/$GOOS_GOARCH/dlv? I'm worried the walking might open us up to a bug where dlv gets installed in the image for the wrong arch.
There was a problem hiding this comment.
Pushed c234551 to address this, please let me know if that's looking any better!
luhring
force-pushed
the
add-debug-flag-to-include-delve
branch
from
5e467b4
to
2cf5015
Compare
Signed-off-by: Christoph Stäbler <[email protected]>
Signed-off-by: Christoph Stäbler <[email protected]>
Signed-off-by: Christoph Stäbler <[email protected]>
Signed-off-by: Dan Luhring <[email protected]>
Signed-off-by: Dan Luhring <[email protected]>
luhring
force-pushed
the
add-debug-flag-to-include-delve
branch
from
e1438e2
to
c234551
Compare
Signed-off-by: Dan Luhring <[email protected]>
luhring
force-pushed
the
add-debug-flag-to-include-delve
branch
from
280d0c4
to
6e8e6b5
Compare
Signed-off-by: Dan Luhring <[email protected]>
luhring
force-pushed
the
add-debug-flag-to-include-delve
branch
from
6e8e6b5
to
49efbef
Compare
Signed-off-by: Dan Luhring <[email protected]>
Signed-off-by: Dan Luhring <[email protected]>
Signed-off-by: Dan Luhring <[email protected]>
luhring
force-pushed
the
add-debug-flag-to-include-delve
branch
5 times, most recently
from
ff65ff7
to
d3c4200
Compare
Signed-off-by: Dan Luhring <[email protected]>
luhring
force-pushed
the
add-debug-flag-to-include-delve
branch
from
d3c4200
to
9a2e1a7
Compare
Picking up the awesome work from #1148, just resolving current conflicts and seeing if we can get this merged.
I've tested this running the built container in Docker Desktop on macOS, using GoLand (running natively on macOS), and it appears to work great.
If there's anything further this PR needs, I'm happy to help out. I'd love to see this
--debugflag inko! 😃cc: @imjasonh