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.

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

cmd/vet: add -cgoinclude check for #include of code in subdirectories #26506

Open
rsc opened this Issue Jul 20, 2018 · 7 comments

Comments

Projects
None yet
6 participants
@rsc
Contributor

rsc commented Jul 20, 2018

The go build model is that all code for a package is in one directory.
The cache relies on this.
Vendoring tools rely on this.
Probably many other things rely on this.

One of the few ways a package can violate this rule is by using
#include in cgo source code to access code in nearby directories.
This appears to work, in that the package builds,
but it doesn't cache properly, doesn't vendor properly, and so on.

It seems reasonable for cmd/vet to notice such #includes and
complain about them, and we could enable that check as one
of the automatic ones run during go test.

Suggested in #26366.

@rsc rsc added this to the Go1.12 milestone Jul 20, 2018

@ysmolsky ysmolsky added help wanted and removed help-wanted labels Jul 20, 2018

@iamoryanmoshe

This comment has been minimized.

Contributor

iamoryanmoshe commented Jul 24, 2018

What's the bullet point for this issue?
Not to allow #include in code if it contains a relative path?

Would love to work on it if @shivakumargn isn't

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jul 24, 2018

Yes, look at the cgo preamble comment and the C/C++ files (including header files) in the directory, and warn about uses of #include "" (not #include <>, <> is always OK) with a relative path that contains at least one path separator.

@iamoryanmoshe

This comment has been minimized.

Contributor

iamoryanmoshe commented Jul 24, 2018

I'm on it!
@ianlancetaylor BTW, why check the files in the directory? I can just throw every include that contains the / char.
Or am I missing something?

@bcmills

This comment has been minimized.

Member

bcmills commented Jul 24, 2018

I can just throw every include that contains the / char.

Includes that fall outside the Go source tree aren't our responsibility to warn about: #include "/usr/include/blah.h" is not usually a good idea, but not for the same reason as #include "subdir/blah.h".

@iamoryanmoshe

This comment has been minimized.

Contributor

iamoryanmoshe commented Jul 24, 2018

OK, so I throw an error only when the include is in a subdir of the project, but not when it is directly in the same dir as the go file, and not when it's outside the src tree.

Got you right?

Should I throw a warning for the third case or just leave it be?

@bcmills

This comment has been minimized.

Member

bcmills commented Jul 26, 2018

If it's outside the source tree, just leave it be. (It's not portable, but that is likely intentional: for example, the Go program may be a command-line tool for managing some aspect of a particular OS or distribution.)

@gopherbot

This comment has been minimized.

gopherbot commented Jul 27, 2018

Change https://golang.org/cl/126375 mentions this issue: vet: check for invalid Cincludes in cgo code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment