Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

proposal: cmd/go: vendor: only copy files that the Go build system recognizes #67233

Open
ianlancetaylor opened this issue May 7, 2024 · 11 comments
Open

proposal: cmd/go: vendor: only copy files that the Go build system recognizes #67233

ianlancetaylor opened this issue May 7, 2024 · 11 comments
Labels
GoCommand cmd/go Proposal
Milestone
Proposal

Comments

@ianlancetaylor
Copy link
Member

Proposal Details

Currently go mod vendor copies all files in a package except:

  • files whose name ends with _test.go
  • files named go.mod and go.sum
  • files whose names ends with .go and contain a go:build ignore constraint

This means that other random files are copied into the vendored directory. For example, the vendored golang.org/x/sys/unix in the main repo includes mkall.sh, mkerrors.sh, and README.md. There is no reason to vendor those files.

I propose that we change go mod vendor to only copy files that go build recognizes. This is files that end with .go, .c, .cxx, .swig, and so forth. Making this change would also require go mod vendor to look for go:embed directives and copy over all files that they mention (for example, the vendored files in GOROOT/src/cmd/vendor/github.com/google/pprof/internal/driver/html).
@gopherbot gopherbot added this to the Proposal milestone May 7, 2024
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals May 7, 2024
@ianlancetaylor ianlancetaylor mentioned this issue May 7, 2024
Closed
@seankhliao
Copy link
Member

given #26366, isn't it just a bug that cmd/go copies more than necessary now?

@ianlancetaylor
Copy link
Member Author

Maybe? I wrote this as a proposal because it's clearly a user-visible change. But if we want to just call the current behavior a bug, that is fine with me.

CC @matloob

@bjorndm

This comment was marked as off-topic.

Sign in to view
@matloob
Copy link
Contributor

matloob commented May 8, 2024

cc @samthanawalla

I think there are some subtleties here that make it more than just a bug. For instance, in the linked issue, #26366 (comment) there's a user who wants to make sure that the LICENSE files are copied in the vendor directory.

@bjorndm

This comment was marked as off-topic.

Sign in to view
@ianlancetaylor
Copy link
Member Author

ianlancetaylor commented May 8, 2024 •
edited
Loading

@bjorndm I'm suggesting that we copy all files that the Go build system recognizes, which includes all files with extensions .c and .h.

@matloob We already copy all files that start with metaPrefixes, including LICENSE; I don't think that should change.

@bjorndm

This comment was marked as off-topic.

Sign in to view
@matloob
Copy link
Contributor

matloob commented May 8, 2024

@ianlancetaylor Ah, got it.

@ianlancetaylor
Copy link
Member Author

@bjorndm Are you saying that that does not work today? That seems like a separate issue from this one.

@bjorndm
Copy link

bjorndm commented May 8, 2024

@ianlancetaylor The file I linked above is an example of the hack that is necessary to get go mod vendor to keep C and H files in subdirectories. I'm here to make sure that this hack keeps on working at least. But you are right, not needing this hack is a separate issue.

@matloob
Copy link
Contributor

matloob commented Jun 11, 2024

This seems reasonable to me.

@seankhliao seankhliao added the GoCommand cmd/go label Feb 22, 2025
@seankhliao seankhliao marked this as a duplicate of #33261 Feb 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
GoCommand cmd/go Proposal
Projects
Proposals
Status: Incoming
Milestone
Proposal
Development

No branches or pull requests
5 participants
@ianlancetaylor @bjorndm @gopherbot @seankhliao @matloob