gokrazy / tools

this repository contains the gok CLI tool of gokrazy
https://gokrazy.org
BSD 3-Clause "New" or "Revised" License
50 stars 27 forks source link

Parse package build flags #23

Closed andig closed 3 years ago

andig commented 3 years ago

Follow-up to #21 which added per-package build args. Unfortunately the implementation did not work as the flags passed to go build where- while not causing an error- effectively not applied.

This PR fixes that by parsing the build flags early and using the parsed flags. go.mod/sum are cleaned.

stapelberg commented 3 years ago

Wouldn’t it also work to put one parameter per line into the file? I’m a bit hesitant to pull shell semantics into the mix.

andig commented 3 years ago

Wouldn’t it also work to put one parameter per line into the file? I’m a bit hesitant to pull shell semantics into the mix.

Only if we enforce parsing the per-line syntax (currently not the case). It does not feel obvious to do that and is a bit more cumbersome creating these files e.g. from Makefile. breakglass was already using shlex so it felt the right thing to do.

I would appreciate if the obvious thing- make single-line expressions work- and shlex performed well in my test.

stapelberg commented 3 years ago

Only if we enforce parsing the per-line syntax (currently not the case)

How do you mean? Can you show an example?

breakglass was already using shlex so it felt the right thing to do

breakglass has no other choice because of how SSH works.

Here, we have a choice, and shell escaping/splitting does not classify as “obvious” to me, so let’s think about this a little :)

andig commented 3 years ago

How do you mean? Can you show an example?

Right now the entire buildflags.txt is passed as blob to the args. We're not doing line-splitting (i.e. I didn't notice this needed be done).

Here, we have a choice, and shell escaping/splitting does not classify as “obvious” to me, so let’s think about this a little :)

Absolutely. I've tested the following:

BUILD_ARGS := -tags='release gokrazy' -ldflags '-X "github.com/andig/evcc/server.Version=$(VERSION)" -X "github.com/andig/evcc/server.Commit=$(SHA)"'

mkdir -p buildflags/github.com/andig/evcc
echo "$(BUILD_ARGS)" > buildflags/github.com/andig/evcc/buildflags.txt
stapelberg commented 3 years ago

Right now the entire buildflags.txt is passed as blob to the args. We're not doing line-splitting (i.e. I didn't notice this needed be done).

Ah, I missed that part part about line splitting during code review :)

BUILD_ARGS := -tags='release gokrazy' -ldflags '-X "github.com/andig/evcc/server.Version=$(VERSION)" -X "github.com/andig/evcc/server.Commit=$(SHA)"'

So if we did line splitting, this would be:

-tags=release gokrazy
-ldflags=-X github.com/andig/evcc/server.Version=$(VERSION) -X github.com/andig/evcc/server.Commit=$(SHA)

If you were to specify these to go build on the shell for testing, you can just quote the entire line:

go build "-ldflags=-X main.Version=bleh -X main.Commit=foo" inject.go && ./inject 

What benefit would shlex give us over the string splitting method?

andig commented 3 years ago

How about this:

    var buildFlags []string
    sc := bufio.NewScanner(strings.NewReader(string(b)))
    for sc.Scan() {
        if flag := strings.TrimSpace(sc.Text()); flag != "" {
            buildFlags = append(buildFlags, flag)
        }
    }
andig commented 3 years ago

Ping @stapelberg better now with build flags per line?