[builder] Add --go-build-flags cli flag #12589

braydonk · 2025-03-09T16:13:04Z

Description

This PR adds the --go-build-flags cli flag which will allow the user to pass additional flags for the go build command used to compile the collector.

While doing the refactor, I also made it so that the default binary name now adds the .exe extension when building for Windows.

Link to tracking issue

Fixes #12588
Fixes #12591

Testing

New unit tests were added to verify the build command args based on the config. A new CompileAndGenerate test case was added as well.

Documentation

A section has been added to the README.

This PR adds the `--go-build-flags` cli flag which will allow the user to pass additional flags for the `go build` command used to compile the collector.

braydonk · 2025-03-09T16:14:52Z

cmd/builder/internal/builder/main_test.go

@@ -113,6 +113,7 @@ var replaceModules = []string{
 func newTestConfig(tb testing.TB) *Config {
 	cfg, err := NewDefaultConfig()
 	require.NoError(tb, err)
+	cfg.Distribution.Name = "test_distribution"


Currently, the integration tests are running with a blank name field, which means the args end up being build -o 'ldflags=-s -w' .... So it's building a binary with the name 'ldflags=-s -w' instead of applying the ldflags. It didn't break any tests but is probably not intentional.

I had removed these because I didn't understand what they did at first, and forgot to add them back once I realized why they were there.

codecov · 2025-03-09T17:08:12Z

Codecov Report

Attention: Patch coverage is 92.30769% with 3 lines in your changes missing coverage. Please review.

Project coverage is 92.21%. Comparing base (7d3e03e) to head (4a52b1e).
Report is 190 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/builder/internal/builder/config.go	90.90%	2 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (92.30%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12589      +/-   ##
==========================================
+ Coverage   92.18%   92.21%   +0.02%     
==========================================
  Files         469      469              
  Lines       25394    25408      +14     
==========================================
+ Hits        23409    23429      +20     
+ Misses       1574     1570       -4     
+ Partials      411      409       -2

☔ 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.

braydonk · 2025-03-10T02:19:48Z

cmd/builder/internal/builder/config.go

+	gcflags := ""
+	executableName := c.Distribution.Name
+	if runtime.GOOS == "windows" {
+		executableName += ".exe"


Codecov seems to think these aren't covered, but I think they are covered by TestGetGoBuildArgs.

the reason you're getting this warning is because the body of the if statement isn't being run in codecov because it's not running in windows. I think this finding should probably be ignored because the test TestGetGoBuildArgs explicitly tests correctly on windows systems

mattsains · 2025-03-10T23:01:50Z

cmd/builder/internal/builder/config.go

+	gcflags := ""
+	executableName := c.Distribution.Name
+	if runtime.GOOS == "windows" {
+		executableName += ".exe"


the reason you're getting this warning is because the body of the if statement isn't being run in codecov because it's not running in windows. I think this finding should probably be ignored because the test TestGetGoBuildArgs explicitly tests correctly on windows systems

github-actions · 2025-03-25T03:21:58Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

mattsains · 2025-03-25T03:25:36Z

@open-telemetry/collector-approvers I think this needs review

github-actions · 2025-04-09T03:23:20Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

mx-psi · 2025-04-09T09:51:29Z

cmd/builder/README.md

+```console
+ocb --config=config.yaml --go-build-flags="-p 32 -buildvcs=false"
+```
+If you provide any flags in `--go-build-flags` that are already configured by the builder automatically, **they will be overridden**:


Is this flexible enough to remove any option passed by default? For example, what if I don't want to pass -trimpath? Could I do this by overriding it with --go-build-flags?

so we used to be able to pass the binary name on the command line as well as a lot of other flags
#11576
But that was deprecated in favor of manifest.yaml config values
I am thinking we should try to keep things in the config/manifest file to the maximum extent possible, and adding command line flags should be an exception that requires justification why it can't be in the yaml

mx-psi · 2025-04-09T09:52:01Z

cmd/builder/README.md

+
+You can provide additional flags to `go build` with the builder flag `--go-build-flags`:
+```console
+ocb --config=config.yaml --go-build-flags="-p 32 -buildvcs=false"


Any advantage of this being a CLI flag vs being in config.yaml?

So I would agree with that approach, except for that we already have --ldflags on the command line
#11996
My question is more if we take this approach (--go-build-flags) if we should deprecate the --ldflags option since --ldflags could be passed with this argument, right?
I would vote moving both this and ldflags to the manifest.yaml in general, though.

jackgopack4

I am not convinced that we should be adding more CLI arguments to ocb, especially when we have recently removed several recently (#11576)
I think there is an argument to be made that this and the ld/gc flags should be moved to a config.yaml section

jackgopack4 · 2025-04-10T15:13:20Z

cmd/builder/README.md

+
+You can provide additional flags to `go build` with the builder flag `--go-build-flags`:
+```console
+ocb --config=config.yaml --go-build-flags="-p 32 -buildvcs=false"


So I would agree with that approach, except for that we already have --ldflags on the command line
#11996
My question is more if we take this approach (--go-build-flags) if we should deprecate the --ldflags option since --ldflags could be passed with this argument, right?
I would vote moving both this and ldflags to the manifest.yaml in general, though.

jackgopack4 · 2025-04-10T15:15:34Z

cmd/builder/README.md

+```console
+ocb --config=config.yaml --go-build-flags="-p 32 -buildvcs=false"
+```
+If you provide any flags in `--go-build-flags` that are already configured by the builder automatically, **they will be overridden**:


so we used to be able to pass the binary name on the command line as well as a lot of other flags
#11576
But that was deprecated in favor of manifest.yaml config values
I am thinking we should try to keep things in the config/manifest file to the maximum extent possible, and adding command line flags should be an exception that requires justification why it can't be in the yaml

github-actions · 2025-04-25T03:26:24Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

braydonk · 2025-04-25T12:30:50Z

I'm just going to close this I think. The reason I wanted to add this is similar to the reason I opened #11453, which is that there are ways I want to be able to build the Collector from ocb that can't be known at config time. However, the reason that issue was dismissed probably applies here too. I am generally opposed to this unilateral move to config over CLI flags, but not willing to fight for it. So at this point I just --skip-compilation and use Go directly.

#12591 which this PR addressed still should be fixed.

DesAWSume · 2025-05-12T03:17:55Z

Looks like this has been merged, could we have an update README or a instruction to build a exe/msi file will work on Windows?

braydonk · 2025-05-12T12:33:40Z

@DesAWSume This wasn't merged. The instructions that exist on the README work the same on Windows, the only difference is you will have to rename the resulting binary to have a .exe extension.

[builder] Add --go-build-flags cli flag

aeabed7

This PR adds the `--go-build-flags` cli flag which will allow the user to pass additional flags for the `go build` command used to compile the collector.

braydonk requested a review from a team as a code owner March 9, 2025 16:13

braydonk requested a review from codeboten March 9, 2025 16:13

braydonk commented Mar 9, 2025

View reviewed changes

braydonk added 3 commits March 9, 2025 12:20

add GCSet and LDSet back

ae33234

I had removed these because I didn't understand what they did at first, and forgot to add them back once I realized why they were there.

also add the clarifying comment

3900b65

fix broken config tests, add build flags compile test

4f74de3

braydonk marked this pull request as draft March 9, 2025 17:00

improve test coverage

9d8ea38

braydonk force-pushed the ocb-build-flags branch from 7525fca to 9d8ea38 Compare March 10, 2025 01:28

add documentation

f5d944e

braydonk marked this pull request as ready for review March 10, 2025 01:38

braydonk added 3 commits March 9, 2025 21:41

fix new build arg test on windows

b4b4c25

fix typo in test expectation

35b6506

fix lint error

4a52b1e

braydonk commented Mar 10, 2025

View reviewed changes

mattsains approved these changes Mar 10, 2025

View reviewed changes

github-actions bot added the Stale label Mar 25, 2025

github-actions bot removed the Stale label Mar 26, 2025

github-actions bot added the Stale label Apr 9, 2025

mx-psi reviewed Apr 9, 2025

View reviewed changes

github-actions bot removed the Stale label Apr 10, 2025

jackgopack4 reviewed Apr 10, 2025

View reviewed changes

github-actions bot added the Stale label Apr 25, 2025

braydonk closed this Apr 25, 2025

[builder] Add --go-build-flags cli flag #12589

[builder] Add --go-build-flags cli flag #12589

Uh oh!

Conversation

braydonk commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Link to tracking issue

Testing

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 25, 2025

Uh oh!

mattsains commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackgopack4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 25, 2025

Uh oh!

braydonk commented Apr 25, 2025

Uh oh!

DesAWSume commented May 12, 2025

Uh oh!

braydonk commented May 12, 2025

Uh oh!

Uh oh!

braydonk commented Mar 9, 2025 •

edited

Loading

codecov bot commented Mar 9, 2025 •

edited

Loading

mattsains commented Mar 25, 2025 •

edited

Loading