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

Add debug flag to include delve #1320

Merged
merged 11 commits into from
Jun 11, 2024

Conversation

luhring
Copy link
Contributor

@luhring luhring commented May 21, 2024

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 --debug flag in ko! 😃

cc: @imjasonh

Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved

// find the delve binary in tmpInstallDir/bin/
delveBinary := ""
err = filepath.WalkDir(filepath.Join(tmpInstallDir, "bin"), func(path string, d fs.DirEntry, err error) error {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have to walk? Don't we know it'll be at tmpInstallDir/bin/dlv?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @imjasonh,
the motivation behind this is as @luhring said in #1320 (comment): for other archs it can be located at different locations/folders.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@luhring luhring May 22, 2024

Choose a reason for hiding this comment

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

Pushed c234551 to address this, please let me know if that's looking any better!

pkg/build/gobuild.go Outdated Show resolved Hide resolved
@luhring luhring force-pushed the add-debug-flag-to-include-delve branch from 5e467b4 to 2cf5015 Compare May 21, 2024 02:02
@creydr
Copy link
Contributor

creydr commented May 21, 2024

Awesome @luhring,
thanks for fixing the merge conflicts (last week I didn't see any when I rebased #1148 🙈 ) and giving this some push again.

creydr and others added 5 commits May 22, 2024 18:30
Signed-off-by: Christoph Stäbler <[email protected]>
Signed-off-by: Christoph Stäbler <[email protected]>
Signed-off-by: Dan Luhring <[email protected]>
@luhring luhring force-pushed the add-debug-flag-to-include-delve branch from e1438e2 to c234551 Compare May 22, 2024 22:31
Signed-off-by: Dan Luhring <[email protected]>
docs/features/debugging.md Outdated Show resolved Hide resolved
@luhring luhring force-pushed the add-debug-flag-to-include-delve branch from 280d0c4 to 6e8e6b5 Compare May 24, 2024 00:20
@luhring luhring force-pushed the add-debug-flag-to-include-delve branch from 6e8e6b5 to 49efbef Compare May 24, 2024 00:26
pkg/build/gobuild.go Outdated Show resolved Hide resolved
imjasonh
imjasonh previously approved these changes Jun 9, 2024
@luhring luhring force-pushed the add-debug-flag-to-include-delve branch 5 times, most recently from ff65ff7 to d3c4200 Compare June 11, 2024 14:10
@luhring luhring force-pushed the add-debug-flag-to-include-delve branch from d3c4200 to 9a2e1a7 Compare June 11, 2024 15:01
@imjasonh imjasonh merged commit 0dcace3 into ko-build:main Jun 11, 2024
18 of 19 checks passed
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