fix(tool): forward -C flag directory to package loading

[Pulkit7070]

Problem

otelc go build -C <dir> does not respect the -C flag. Package patterns like . or ./... are resolved against the process working directory instead of <dir> because packages.Config.Dir was never set.

Steps to reproduce:

1. Create a Go module in /some/dir
2. Run otelc go build -C /some/dir ./... from a different directory
3. Package loading fails or instruments the wrong packages

Fixes #507

Solution

Add LoadPackagesInDir to the pkgload package. It accepts an explicit dir string and sets packages.Config.Dir, so go/packages resolves patterns relative to that directory.

In getBuildPackages, extract the -C flag value from the build args using the new extractFlagValue helper and forward it to every LoadPackagesInDir call.

Existing callers (ResolvePackageName, ResolveExportFiles) continue to use LoadPackages, which delegates to LoadPackagesInDir with an empty dir.

Changes

• tool/internal/pkgload/pkgload.go: add LoadPackagesInDir(ctx, dir, mode, buildFlags, patterns...). LoadPackages is a thin wrapper passing empty dir.
• tool/internal/setup/setup.go: add extractFlagValue helper; update getBuildPackages to extract -C and pass it to LoadPackagesInDir.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Pulkit7070 / name: Pulkit Saraf (081cf15)

[codecov]

Codecov Report

❌ Patch coverage is 61.29032% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.28%. Comparing base (3e45b48) to head (c08fc9e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
tool/internal/setup/setup.go	57.14%	10 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           main     #518       +/-   ##
=========================================
+ Coverage      0   63.28%   +63.28%     
=========================================
  Files         0       62       +62     
  Lines         0     4827     +4827     
=========================================
+ Hits          0     3055     +3055     
- Misses        0     1519     +1519     
- Partials      0      253      +253     

Flag	Coverage Δ
tool	63.28% <61.29%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[y1yang0]

LGTM, this is surprisingly clean and elegant. Please also add an integration test

[amazingakai]

This currently does not work:

otelc go build -C ./somedir

because we pass -C after -a -x -n inside listBuildPlan.

From the debug log:

new="[go build -a -x -n -C ./somedir]"

But go requires -C to appear before build flags/packages, so this becomes invalid.

Would it be simpler to consume the -C flag early and just chdir into it at command start (with a deferred restore of the original cwd)?

[kakkoyun]

This currently does not work:

otelc go build -C ./somedir

because we pass -C after -a -x -n inside listBuildPlan.

From the debug log:

new="[go build -a -x -n -C ./somedir]"

But go requires -C to appear before build flags/packages, so this becomes invalid.

Would it be simpler to consume the -C flag early and just chdir into it at command start (with a deferred restore of the original cwd)?

@Pulkit7070 concerns of @amazingakai needs to be addressed here.

[Pulkit7070]

Thanks for the thorough reviews. Both points are well-taken and I've addressed them in the latest push.

@amazingakai - you're right that the cfg.Dir approach was incomplete. The deeper problem is that listBuildPlan in find.go prepends -a -x -n to the remaining args, so -C dir would end up in the wrong position (go build -a -x -n -C dir ./...) and the go tool would reject it. The fix now uses os.Chdir() instead, and a new consumeCFlag helper strips -C dir from the args before they reach listBuildPlan, so the subprocess never sees the flag at all. The working directory change covers both the go/packages resolution step and the listBuildPlan dry-run. getBuildPackages also handles os.Chdir() for the case where it is called with the original args from the GoBuild cleanup defer.

@y1yang0 - LoadPackagesInDir is fully reverted. pkgload.go is back to its original form with only LoadPackages, ResolvePackageName, and ResolveExportFiles. The -C handling lives entirely in setup.go where the flag is consumed.

Let me know if anything looks off - happy to iterate.

[kakkoyun]

I merged #468 (first come, first served). This needs another rebase.

And let's wait for @amazingakai's review.

[Pulkit7070]

Rebased on main. Also squashed the messy history (reverts, CLA retriggers, merge commit) down to a single clean commit.

[y1yang0]

But why not fix listBuildPlan by inserting -C after go build to solve this problem?

[Pulkit7070]

Good question. Fixing listBuildPlan alone is not sufficient because the -C flag affects two independent layers:

1. go/packages resolution - when getBuildPackages calls packages.Load, it resolves patterns like ./... relative to the process working directory. Repositioning -C in listBuildPlan does not help here at all.
2. listBuildPlan dry-run - the subprocess command needs -C in the correct position.

To cover both with the cfg.Dir approach, we would need to thread the extracted dir value into every packages.Config construction, not just listBuildPlan.

os.Chdir handles both layers with a single change: the process working directory shifts, so go/packages resolves correctly and the dry-run subprocess inherits the same cwd. consumeCFlag strips -C from args so listBuildPlan never sees it and the subprocess flag ordering stays valid.

Also, amazingakai explicitly suggested the os.Chdir approach in their CHANGES_REQUESTED review, which is still open. Happy to switch if you and amazingakai align on the listBuildPlan approach instead.

[y1yang0]

But if we are to use os.chdir directly, we would essentially need to mirror the behavior of the Go toolchain as implemented here:

https://github.com/golang/go/blob/03d1f8efc82678bba84d5e50d9144cfc6847a1f9/src/cmd/go/main.go#L383

[Pulkit7070]

You are right. Go's handleChdirFlag is strictly positional - it only looks for -C at the argument immediately after the subcommand, and removes it from os.Args so a reinvoked child toolchain does not process it again. Our consumeCFlag scans the full arg list, which does not match those semantics.

Given this, I think the cleaner fix is the one you originally suggested: fix listBuildPlan to insert -C dir right after go build, and set cfg.Dir on the packages.Config. No process-global chdir, no positional constraint to enforce, and the behavior is explicit at each call site.

Would you and @amazingakai prefer we switch to that approach? Happy to implement it - just want both reviewers to align before we push another redesign.

[y1yang0]

Apologies for any confusion. I'd like to clarify my thought process. Initially, it seemed to me that both potential fixes were reasonable:

• The os.chdir approach, where subsequent commands are unaware of the -C flag. This seemed simpler.
• Passing -C to LoadPackages and fixing the argument order in listBuildPlan (go build -C dir {remaining}). This seemed more explicit and self-contained.

However, after reviewing the Go toolchain's source code, I see its logic is essentially to consume the -C flag and then call os.Chdir. This strongly suggests that the first approach is indeed the more correct and idiomatic one.

I'm leaning this way now. Do other reviewers have any further thoughts on this?

[Pulkit7070]

Thanks for the clarification. Agreed - Go's own toolchain doing os.Chdir is a strong signal that it is the right approach.

The one remaining gap in our current implementation: our consumeCFlag scans the entire args list for -C, but Go's handleChdirFlag only looks for it at the specific position right after the subcommand. We should tighten that to match Go's positional semantics before merging.

Waiting on @amazingakai to weigh in, then we will implement with that correction.

[amazingakai]

@Pulkit7070 I think it completely makes sense to match go's behavior here. We should consume -C only if it appears as the first argument after otelc go build/install or otelc setup. Also, after looking at the go toolchain's code I realized we don't need to restore to the original directory with defer, changing the process' dir doesn't change the shell's dir, so restoring to the original dir would do nothing here.

Also, something probably went wrong with the rebase here, this PR is undoing some changes I did in #468 and also removing some unrelated comments. Please fix those.

[Pulkit7070]

Done. Three things addressed in the latest push:

1. Positional constraint: replaced consumeCFlag (full scan) with consumeCFlagPositional that only consumes -C when it is the first argument, matching Go toolchain semantics.
2. No defer restore: removed the defer os.Chdir(orig) as you noted it has no effect on the parent shell.
3. fix(tool): otelc setup command failing to run #468 regression fixed: the previous squash was sitting on top of pre-fix(tool): otelc setup command failing to run #468 code. Rebased cleanly on current main - all of #468s changes are intact and the only diff now is +26 lines in setup.go and +115 lines in setup_test.go.

[amazingakai]

We also require consumeCFlagPositional in GoBuild, otherwise the build fails, but this could be a little tricky.
We have to handle the following cases:

• otelc setup -C ... -- already handled
• otelc go build -C ... -- after consuming the -C flag we also have to trim it from the args. We might need to change the signature of Setup a little here and accept args + flags instead of cmd. @y1yang0, would appreciate your opinion here as well.
• go build also supports both go -C ./dir build and go build -C ./dir, so we have to check both cases and allow -C to appear immediately before or after the build/install arg.

[amazingakai]

https://github.com/golang/go/blob/03d1f8efc82678bba84d5e50d9144cfc6847a1f9/src/cmd/go/main.go#L395
Also supports --C

[Pulkit7070]

Good catch. Tracing through the two cases:

go build -C dir ./...: cmd.Args().Tail() = [-C, dir, ./...], consumeCFlagPositional picks it up at position 0 - already handled.

go -C dir build ./...: GoBuild fails immediately at the cmd.Args().First() != "build" check before Setup is even called. And even if we fixed that, Setup does cmd.Args().Tail() which gives ["dir", "build", "./..."] - the -C is gone.

The fix needs to happen in GoBuild before any arg inspection. Since cmd cannot be mutated, the cleanest path is the one you suggested: change Setup to accept an explicit args slice instead of cmd, so GoBuild can pre-process -C in both positions, chdir, strip it, then pass the cleaned args down.

Rough shape:

consumeCFlagFromArgs would check position 0 for -C (before subcommand) and also position 1 for the go build -C case after subcommand trimming.

Does this match what you had in mind? Happy to implement once you and @y1yang0 agree on the shape.