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

Go: Add -gcflags to std_go_args #18615

Merged
merged 1 commit into from
Oct 25, 2024
Merged

Conversation

Neved4
Copy link
Contributor

@Neved4 Neved4 commented Oct 23, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This change adds support for compiler flags (gcflags) as standard arguments in Go builds, enabling better control over compilation behavior.

So, instead of:

*std_go_args(
  ldflags: "-w -s"
) + ["-gcflags=all=-l -B -wb=false"]

You can pass them as arguments:

*std_go_args(
  ldflags: "-w -s",
  gcflags: "all=-l -B -wb=false",
)

@Neved4 Neved4 marked this pull request as ready for review October 23, 2024 18:12
Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

Seems reasonable in case we need to add standard gcflags in the future.

@Neved4 Neved4 changed the title Add -gcflags to std_go_args Go: Add -gcflags to std_go_args Oct 24, 2024
@Neved4
Copy link
Contributor Author

Neved4 commented Oct 24, 2024

Some additional context:

  1. Wasn't sure how to test the changes, since std_go_args didn't seem to include any tests.
  2. There's a lot of interesting flags under go help build that seem relevant to formula (e.g. -asmflags, -gccgoflags, -covermode, -pgo, -tags). These might be better suited for a combined PR, or grouped in separate ones.
  3. Whether it makes sense to wrap all of them is open for consideration. Adding some could make it more convenient for maintainers, placing them under the same unified syntax. We could use std_go_args for everything, or favor hand-rolled array concatenation as above. Ultimately, this decision is up to brew devs, you guys know what makes the most sense.

Cheers! 🧡 🍺

@Bo98
Copy link
Member

Bo98 commented Oct 24, 2024

Usually when it comes to extra arguments the preferred way to do it is e.g.

system "go", "build", *std_go_args, "-extraflag", "-extraflag2"

The reason ldflags was different is because if we specified a default ldflag within std_go_args then this:

system "go", "build", *std_go_args, "-ldflags=-abc"

would actually override the ldflags in std_go_args rather than append which would be problematic. We just never actually followed through and added any default ldflags so far, so it hasn't mattered yet.

So I'm ok adding arguments like this for flags that behave like -ldflags, i.e. flags that themselves take in a list of flags that would make sense to merge with a theoretical default list of flags.

@Neved4
Copy link
Contributor Author

Neved4 commented Oct 24, 2024

Sounds great, 100% agree

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Seems reasonable, thanks!

@MikeMcQuaid MikeMcQuaid merged commit bbe5a85 into Homebrew:master Oct 25, 2024
27 checks passed
@Neved4
Copy link
Contributor Author

Neved4 commented Oct 25, 2024

Thank you!

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