asm: Pass package path with -p

[fmeum]

This is required to produce linkable objects as of Go 1.19.

Fixes #3199

[fmeum]

@abhinav Could you test?

[sywhang]

@abhinav is on vacation till a couple weeks later, so I took over the testing here at Uber. Can confirm that the fix works (minus the duplicate build errors part - I just got around that issue by renaming the <1.19 version's function to something else just for testing's sake).

[sluongng]

For some reason, I do get duplicate definition build errors though - I'm probably using //go:build wrong. Do you know how I can fix it?

I tried to reproduce this with normal go build command

~/test> cat main.go
package main

import "fmt"

func main() {
    fmt.Println(Version)
}

~/test> cat lib_go1.go
//go:build !go1.19
// +build !go1.19

package main

var Version = "1.18"

~/test> cat lib_go119.go
//go:build go1.19
// +build go1.19

package main

var Version = "1.19"

~/test> cat go.mod
module temp

go 1.18

~/test> go version
go version go1.18.3 darwin/arm64

And then run this

~/test> GOCACHE=$(mktemp -d) go build -x .

...
/opt/homebrew/Cellar/go/1.18.3/libexec/pkg/tool/darwin_arm64/compile -o $WORK/b001/_pkg_.a -trimpath "$WORK/b001=>" -p main -lang=go1.18 -complete -buildid UPD40Vf15HmsGAFN49LI/UPD40Vf15HmsGAFN49LI -goversion go1.18.3 -shared -c=4 -nolocalimports -importcfg $WORK/b001/importcfg -pack ./lib_go1.go ./main.go
...

Note how lib_go1.go was included and not lib_go119.go.

So it seems like go tool compile is depending on go build to filter out the source file before hand?
And we might need to replicate that logic with rules_go?

[sluongng]

It seems like this will affect #3212 as well.

There are a few ways we can go about this:

• Build tooling to support different implementation for different goSdk versions

• Commit to a non-backward compatible upgrade

The second option seems to be a lot easier:

• Make v0.34.0 to support Go 1.18 minimum, include nogo: instantiate type info for generic types when running under Go >=1.18 #3212 in the release

• Make v0.35.0 to support Go 1.19 minimum (only after Go 1.19 has come out), include this PR in the release.

[sluongng]

cc: @thempatel who have been doing some work around build constraints over in Gazelle repo.

[fmeum]

Thanks for digging in!

It's unfortunate that build constraints are handled by a preprocessing step we would have to implement ourselves. While we can certainly just do backwards incompatible changes in rules_go while we aren't at v1 yet, users that deal with multiple Go SDK versions at the same time are still affected by this.

I see two ways we could handle this:

• Implement our own build tags parsing - a lot of work, hard to maintain.
• Represent Go versions as toolchain constraints. We could define one constraint per minor version and replace build constraints with ordinary select.

@sluongng @achew22 What do you think? Something like this would look pretty usable to me:

go_binary(
    name = "foo",
    srcs = select({
        "@io_bazel_rules_go//go/constraints:go1.19": ["compiled_on_go1.19_and_up.go"],
        "//conditions:default": ["compiled_on_go1.18_and_lower.go"],
    }),
)

[sluongng]

I think thats the sensible solution, but I don't like it personally. 😆
It's possible, does not means we should.

Reason is simply "additional complexity" on top of existing platform + cgo on/off matrixes.
And to support this long term, the parser inside gazelle would also need to handle this properly. (yes, we already DO have a parser, see bazel-contrib/bazel-gazelle#1243).
Between both rules_go and gazelle, the active maintainers is quite limitted so I would vote to keep it simple so that we can focus on more important things.

Imo we only need to provide a release cadence where 1 version that is backward compatible with the old GoSDK. Meaning that the moment 1.19 hits release, we can safely say that 1.17 is no longer supported and move to 1.18, then shortly move to 1.19 in the version after that, with possible cherry-pick backporting of critical bugs via patch releases.

Folks who wish to adopt latest Go version earlier (i.e. during RC releases) should have the capacity to patch rules_go with the content of this PR to make it work.

[fmeum]

Sounds reasonable to me in general.

But don't we have the special problem here that we need this change to support 1.19 but the change fails with versions before 1.19? This would be much simpler if asm had -p already in 1.18 as then we could just add support for 1.19 at the same time we drop support for 1.17.

[sluongng]

See https://github.com/bazelbuild/rules_go/wiki/Deprecation-schedule#supported-go-versions

The Go Team provides support for the current and previous minor versions of Go. For example, if Go 1.15 is the current version, then 1.15 and 1.14 are supported. Only supported versions receive bug fixes and security updates, so users are strongly encouraged to update to those versions.

Each minor version of rules_go only supports the minor versions of Go that were supported at the time of release. rules_go may break compatibility with older versions.

So I would suggest these:

1. We remove the multi-version support and leave this PR open for now.
2. Add a description at top guiding people on how to patch their rules_go if they wish to use 1.19 early.
3. Complete and merge nogo: instantiate type info for generic types when running under Go >=1.18 #3212 and create a new release for 1.18 compatibility. In which we announce next release to be on the same date with Go 1.19
4. When Go 1.19 get released, merge this and release a new rules_go version targeting 1.19 onward.

I would love to hear other maintainers opinion on this as well. 🤔

[fmeum]

@sywhang @sluongng I pushed a new version with a runtime check that should allow us to merge this right away. PTAL.

[sluongng]

Look obviously correct to me. 👍

[martinbaillie]

Would it be possible to cut an official release that has this in it? 🙏🏻

[sywhang]

https://github.com/bazelbuild/rules_go/releases/tag/v0.34.0 has this fix in it.

[martinbaillie]

So it does. My apologies!