[golist] PR #17: Replace Go internals with go/build

[Elliott Sales de Andrade]

qulogic opened a new pull-request against the project: `golist` that you are following:
``
Replace Go internals with go/build
``

To reply, visit the link below or just reply to this email
https://pagure.io/golist/pull-request/17

[Elliott Sales de Andrade]

qulogic commented on the pull-request: `Replace Go internals with go/build` that you are
following:
``
So for tests, I searched the specs for all that define`goipath` and cloned all of them
(note this is _not_ all Go packages.) Then ran `golist` as packaged and `golist` from this
PR and compared the results for all possible arguments.

Current `golist` crashes on:

* github.com/gengo/grpc-gateway
* github.com/shurcooL/githubql

but this PR works again.

There are only 2 differences out of 510 packages with `goipath` (out of approximately 642
that mention golang at all):

* github.com/cespare/xxhash
* github.com/google/go-github

Both of these report themselves in the `--imported` section, but the new one includes the
version (i.e., `github.com/cespare/xxhash` -> `github.com/cespare/xxhash/v2`). I assume
this might be a problem, since we don't add it in the provides, but I don't know
for sure. Can you confirm @nim?
``

To reply, visit the link below or just reply to this email
https://pagure.io/golist/pull-request/17

[Elliott Sales de Andrade]

qulogic commented on the pull-request: `Replace Go internals with go/build` that you are
following:
``
As a note, the original code ran a resolve on the test imports, but not the regular
imports. I tried adding a resolve on the regular imports as well, but it doesn't help.
I also tried setting `GO111MODULE=on`, and while it does stop it from erroring, it still
returns the same `/v2` import path back.
``

To reply, visit the link below or just reply to this email
https://pagure.io/golist/pull-request/17

[Nicolas Mailhot]

nim commented on the pull-request: `Replace Go internals with go/build` that you are
following:
``
The v2 is module versioning. That's something we need to support to have working Go
packaging. It's good that your PR starts picking it up when the previous code does
not.

However you are right that just doing it that way is going to break things. It needs
changes in other parts of `golist` *and* in the macro code (and the current go-rpm-macro
code should be in a good enough shape to allow those changes but that ’s not what it in
Fedora).

Looking at how `github.com/cespare/xxhash` handled the module transition, assuming it is
representative of the conversion of a real-world codebase to modules, here is the list of
the things I see needing doing (paper analysis, real work implementation and testing is
needed)

- we have the new module descriptors. That means `golist` needs to include the `go.mod`
file as part of the project files, otherwise it will not end up in the -devel package and
rpm will (rightfully) ignore it when generating deps

- then once the `go.mod` file is in the devel package we need to check the golist call
that generates the package provides actually sees it (should probably just work with the
code in go-rpm-macros and will almost certainly fail in some case with the code in Fedora
because some of the approximations I fixed this year and which are still in the Fedora
macro code will definitely hurt module parsing)

- then we have the fact that modules break the identity between the URL and the import
path. The code is still at `https://github.com/cespare/xxhash` but the logical name is now
`github.com/cespare/xxhash/v2`. No matter this is a case that is handled nicely by
go-rpm-macros, that just means you need to declare `github.com/cespare/xxhash/v2` as 
`goipath` and  `https://github.com/cespare/xxhash` as `forgeurl`

- the module says v2 depends on v1 which means you need a separate v1 spec and package
with just  `github.com/cespare/xxhash/` `goipath` (and be careful to pull from the V1
banch not the v2 one)

- but, what happens when a single git repo ships several `go.mod` files with a logical
name that has nothing to do with the URL path ? I suspect things will break badly. the
latest go-rpm-macros code is a lot more flexible, but it still assumes some form of
identity between the filesystem layout and the import layout. How badly will this identity
break with modules? I have no idea.

- and then you have the version qualifiers in the module files, we need to map them rpm
side one way or another


``

To reply, visit the link below or just reply to this email
https://pagure.io/golist/pull-request/17

[Nicolas Mailhot]

nim commented on the pull-request: `Replace Go internals with go/build` that you are
following:
``
Ok, after more work evolving to module support seems a bit simpler than I though, because
go mod need requires people to clean up their mess and that's also the assumption of
the current go-rpm-macros.

Though it will definitely require the coding of a helper in go, since the json structure
go mod edit outputs is pretty much unworkable in shell, lua is not available at this stage
of the rpm build, and I don't think we need to inject a fourth language in our go
tooling (after lua+shell due to how rpm works, and golang for golist).

The 1000-dollar question is how much this new helper can subsume current golist functions,
or if we end up with 2 helpers because go module is almost  there for our needs but not
completely.
``

To reply, visit the link below or just reply to this email
https://pagure.io/golist/pull-request/17

[Elliott Sales de Andrade]

qulogic commented on the pull-request: `Replace Go internals with go/build` that you are
following:
``
I'm going to merge this, as those two packages are failing due to the way they use Go
modules, and getting testing working everywhere is more important. They can just be
patched to work right in the meantime. Of course, as more things move to modules, this
will be harder to do, but I guess you're working on that or we can get to it later.
``

To reply, visit the link below or just reply to this email
https://pagure.io/golist/pull-request/17

[Elliott Sales de Andrade]

qulogic merged a pull-request against the project: `golist` that you are following.

Merged pull-request:

``
Replace Go internals with go/build
``

https://pagure.io/golist/pull-request/17