Do you have an example?

Running

golist --provided --tests --package-path github.com/osbuild/osbuild-composer

Without fix:

github.com/osbuild/osbuild-composer/internal/crypt
github.com/osbuild/osbuild-composer/internal/distro
github.com/osbuild/osbuild-composer/internal/pipeline

With fix:

github.com/osbuild/osbuild-composer/internal/crypt
github.com/osbuild/osbuild-composer/internal/distro
github.com/osbuild/osbuild-composer/internal/distro/fedora30
github.com/osbuild/osbuild-composer/internal/distro/rhel82
github.com/osbuild/osbuild-composer/internal/jobqueue
github.com/osbuild/osbuild-composer/internal/pipeline
github.com/osbuild/osbuild-composer/internal/weldr

That would wreck havoc on all package BRs since internal is a magic keyword that forbids exposing the result to other Go packages.

Returning anything internal in golist must be isolated with an '--internal' optional switch (and %gocheck should probably use this flag, once it is created. The R and BR part definitely should not)

(And I see golist is already returning some internal stuff, that can not end well)

But if I undrestand things correctly this is not connected with internal keyword in any way. It wouldn't list all packages even if they're not internal.

@obudai a lot of Go projects use internal in the package path to mean the same thing as the internal keyword. Welcome to the wonderful land of underspecified Go behaviour.

Therefore, to be safe, golist should filter anything with internal in the path, unless requested explicitly with a specific switch. (and upstreams that use internal to mean something else than internal should probably rename their Go package now that internal as a package name has been used as internal by many Go projects)

I know, it's clear as mud.

@nim TBH I don't think that we should be creating our own dialect of Go(even from tooling perspective). What you are suggesting is basically it. We should respect upstream specs of the Go with all its warts and possibly work with upstream to clean up or add things that make packaging hard for us.

I think that you shouldn't filter dependency list by default on some more or less arbitrary keyword(that is AFAIK not even mentioned in the guidelines). That doesn't mean that there shouldn't be feature that would filter them base on some keyword/regular expression. I can imagine that being useful in certain scenarios.

@jcajka I agree from a theoretical POW. I was just pointing out that from a practical POW, asking all the Go parts we package to adhere strictly to the specs, means dealing with a lot of build failures.

internal is known to be (mis)used by some projects. Do we want to ignore this (mis)use and just deal with failing builds whenever exposing internal makes things break?

@nim I have assumed that this has been transparently handled in the new macros. I guess there should be some way whatever in the macros or document it in the guidelines that will more or less handle this transparently. Also I would expect that working with upstreams should be mentioned. I'm aware that this might pose fairly non-trivial feat.

Do you have list of packages that contain this in Fedora?

@jcajka the macros do not add any policy over golist output. Any policy is manual use of override switches by packagers in their spec

As for upstreams… you realise that telling someone that named a package tree root internal that it's not internal for third party tooling is not going very far… As for actual examples, the cznic package set comes to mind (don't remember if it's one of those doing the correct go language thing or not, it was one of the packages I remember working on when different upstream interpretations of internal collided)

From a purely semantical POW, projects that use internal to mean something else than internal are the ones asking for trouble

@obudai a lot of Go projects use internal in the package path to mean the same thing as the internal keyword. Welcome to the wonderful land of underspecified Go behaviour.

I don't understand where you get the idea that this is some sort of hack that non-standard-library packages are taking advantage of. The use of internal was described in a design document that has been in use since Go 1.4.

Therefore, to be safe, golist should filter anything with internal in the path, unless requested explicitly with a specific switch. (and upstreams that use internal to mean something else than internal should probably rename their Go package now that internal as a package name has been used as internal by many Go projects)

There is no other way for internal to be interpreted, so I don't know what you mean about other upstreams.

I do agree that if golist is specifying an internal path as something that goes in Provides, then that's a bug and it should be fixed.

I know, it's clear as mud.

It is clear; you're confused.

Coming back to what this PR is actually about, there may be one more case of TestGoFiles that you haven't changed to add XTestGoFiles in BuildArtifact, though I'm not sure we use that for anything.

This would affect approximately 200 packages. Since this determines which test files to run, it could have some possible consequences for the build, but probably not the final package.

I set up a copr and more than half of those failed. However, many of them may already be failing, and, more importantly, the failures actually seem to be problems on copr. I will have to wait for it to be fixed to retry.

This breaks a few things, e.g., golang-github-pquerna-otp , golang-github-opencontainers-image-spec or golang-github-onsi-gomega .

This is because imports from the newly added tests are not listed. You will need to add XTestImports alongside any TestImports usage to get this correct (then we need to update all these packages to match.)

Oh, and of course, that's what the recently merged PR#26 does, so there's no need to modify this PR to do it too (unless you want to rebase.)