asticode / go-astilectron-bundler

Bundle your Astilectron app with ease
MIT License
127 stars 67 forks source link

ldflags broken? #43

Closed frioux closed 5 years ago

frioux commented 5 years ago

82d548aa540b3fa25add9fca24e81cb2cdbc63cf apparently broken ldflags entirely; even after c9219b8cd01bacd440e9cab950ea32d6d7cebf23 I get:

panic: assignment to entry in nil map

goroutine 1 [running]:
github.com/asticode/go-astilectron-bundler.LDFlags.Merge(0x0, 0xc000083560)
        /home/frew/go/src/github.com/asticode/go-astilectron-bundler/ldflags.go:36 +0x12a
main.main()
        /home/frew/go/src/github.com/asticode/go-astilectron-bundler/astilectron-bundler/main.go:90 +0x387

I'm going to configure our build system to just use our fork for the moment; I don't really understand the motivation of the first change, but a quick fix is:

diff --git a/astilectron-bundler/main.go b/astilectron-bundler/main.go
index 3136b2e..5458477 100644
--- a/astilectron-bundler/main.go
+++ b/astilectron-bundler/main.go
@@ -87,6 +87,9 @@ func main() {
        }

        // Flags
+       if c.LDFlags == nil {
+               c.LDFlags = astibundler.LDFlags(make(map[string][]string))
+       }
        c.LDFlags.Merge(astibundler.LDFlags(ldflags))

        // Build bundler

If you're happy with the above I can submit a PR.

I am curious though, what is the benefit of merging this way?

asticode commented 5 years ago

I'm happy with the change, I should have thought about it. You can submit a PR.

The benefit of merging this way is to allow passing custom ldflags through the json configuration file as well. Otherwise, the ldflags passed through the binary flags would overwrite the ones in the configuration file. Does that make sense?

frioux commented 5 years ago

Ah, I'd totally forgotten about the config file thing. Thanks!