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

Adds the possibility to configure go build flags #51

Merged
merged 5 commits into from
Nov 16, 2022

Conversation

thgruiz
Copy link
Contributor

@thgruiz thgruiz commented Oct 20, 2022

to use dlv with the generated binary, as described here: #43

@CLAassistant
Copy link

CLAassistant commented Oct 20, 2022

CLA assistant check
All committers have signed the CLA.

builder.go Outdated Show resolved Hide resolved
@mstoykov
Copy link
Contributor

Hi @thgruiz, thanks for the PR and sorry for the slow response, this seems to have fallen between the cracks :(

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

Sorry for this taking so long, we are about to release a new k6 version and I keep forgetting to look at this.

Comment on lines +91 to +92
// trim debug symbols by default
buildFlags := b.osEnvOrDefaultValue("XK6_BUILD_FLAGS", "-ldflags=-w -s")
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not certain if we don't want to not have this as the default 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

But maybe leave that for a separate PR later as I guess we will need to discuss it with the team more 🤷 . I personally don't think the extra space is a problem, but maybe there are other reasons to trim them by default

builder.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

I did test it locally and seems to work perfectly 👏

So if we can just do the requested changes I think we can merge this

WDYT @imiric

thgruiz and others added 2 commits November 3, 2022 18:42
Co-authored-by: Mihail Stoykov <312246+mstoykov@users.noreply.github.com>
…any build flags

Co-authored-by: Mihail Stoykov <312246+mstoykov@users.noreply.github.com>
@thgruiz
Copy link
Contributor Author

thgruiz commented Nov 3, 2022

I have committed to the suggestions you gave, @mstoykov

Thanks for the reviews and comments! (sorry for my delay, though.)

Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM to me 🎉

Thanks for all the work 🙇

Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Thanks @thgruiz! 👏 This now closes #4, #43 and #48 🎉

I would prefer to see some tests, preferably E2E by setting XK6_BUILD_FLAGS, but that goes into #13 territory, so we could tackle it later. But now that buildCommandArgs() is a standalone function, it should be straightforward to add some unit tests for it. I won't block this for merging, but would appreciate it if you added some. 🙏

Comment on lines +276 to +281
// buildComandArgs parses the build flags passed by environment variable XK6_BUILD_FLAGS
// or the default values when no value for it is given
// so we may pass args separately to newCommand()
func buildComandArgs(buildFlags, absOutputFile string) (buildFlagsSlice []string) {

buildFlagsSlice = make([]string, 0, 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// buildComandArgs parses the build flags passed by environment variable XK6_BUILD_FLAGS
// or the default values when no value for it is given
// so we may pass args separately to newCommand()
func buildComandArgs(buildFlags, absOutputFile string) (buildFlagsSlice []string) {
buildFlagsSlice = make([]string, 0, 10)
// buildCommandArgs parses the build flags passed by environment variable XK6_BUILD_FLAGS
// or the default values when no value for it is given
// so we may pass args separately to newCommand()
func buildCommandArgs(buildFlags, absOutputFile string) (buildFlagsSlice []string) {
buildFlagsSlice = make([]string, 0, 10)

@imiric imiric changed the title Adds the possibility to configure ldflags on builder Adds the possibility to configure go build flags Nov 11, 2022
@imiric
Copy link
Contributor

imiric commented Nov 16, 2022

Merging this as is, since it fixes some important issues. Thanks for your work @thgruiz!

We'll fix the remaining issues and maybe add tests for this as well.

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.

Add -buildvcs=false build option Allow compiling with debug info Pass go build arguments from env var
4 participants