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.

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/go: can't include C files when building package from named list of files #40570

Closed
nathan-fiscaletti opened this issue Aug 4, 2020 · 19 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@nathan-fiscaletti
Copy link
Contributor

What version of Go are you using (go version)?

$ go version
go version go1.13 darwin/amd64

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/nathanf/Library/Caches/go-build"
GOENV="/Users/nathanf/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/nathanf/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/w3/_t_vv2jd6tb1hvj_m7jffg940000gn/T/go-build101759856=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I seem to have an issue with Go where I'm unable to compile a C library that depends on C files if i want to only include a smaller set of the files.

I've put together this example to hopefully illustrate my issue.

  1. test.c

    #include <stdio.h>
    
    void TestFunc() {
        printf("%s\n", "This is a message from C");
    }
  2. test.h

    void TestFunc();
  3. test.go

    package main
    
    // #include "test.h"
    import "C"
    
    func main() {
        C.TestFunc()
    }
  4. test2.go

    package main
    
    func Test2() {
        // This file exists only so that i have something to exclude.
    }

When I run the following, it compiles just fine:

$ go build -o test.a -buildmode=c-archive

However, when I try to build a smaller version that only includes one of the go files, it complains.

$ go build -o test.a -buildmode=c-archive test.go test.c
can't load package: named files must be .go files: test.c

If I don't include the test.c file, I get missing symbols.

$ go build -o test.a -buildmode=c-archive test.go
# command-line-arguments
Undefined symbols for architecture x86_64:
  "_TestFunc", referenced from:
      __cgo_214b47eb264c_Cfunc_TestFunc in _x002.o
     (maybe you meant: __cgo_214b47eb264c_Cfunc_TestFunc)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

How can I make smaller builds of my library while, apparently, being unable to include the C files that are required for it to function?

@ianlancetaylor ianlancetaylor changed the title Can't include C files in named files when using go build -buildmode=c-archive cmd/go: can't include C files in named files when using go build -buildmode=c-archive Aug 4, 2020
@ianlancetaylor
Copy link
Contributor

As far as I know, the go tool doesn't currently provide a way to do what you want.

Of course, you can do something like this:

go build -o test.a -mode=c-archive
cc -c -o testc.o test.c
ar rs test.a testc.o

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 4, 2020
@toothrot toothrot added this to the Backlog milestone Aug 4, 2020
@jayconrod
Copy link
Contributor

Usually, this would be solved with build constraints. You could add a line to test2.go like this:

// +build extras

package main

func Test2() {
    // This file exists only so that i have something to exclude.
}

Then build the package with:

go build -tags=extras .

Does that work for you?

@jayconrod jayconrod added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 11, 2020
@nathan-fiscaletti
Copy link
Contributor Author

@jayconrod for the example above that would technically work, however in my real-world example I would need to be able to tag C files as well since there are a few I would need to exclude along with the Go files. Would adding a similar build-tag comment to the top of a C file and then calling go build without any named files accomplish the same result?

@jayconrod
Copy link
Contributor

Would adding a similar build-tag comment to the top of a C file and then calling go build without any named files accomplish the same result?

Yes. You can run go list -json to confirm whether files will be included in a package.

@nathan-fiscaletti
Copy link
Contributor Author

So it seems like that solves my problem then. However, the ability to add named C files to the go build command is still a feature some might find worth using. That being the case, would it make sense to leave this issue open in case someone wants to submit a PR that adds support for the feature?

@jayconrod jayconrod changed the title cmd/go: can't include C files in named files when using go build -buildmode=c-archive cmd/go: can't include C files when building package from named list of files Aug 13, 2020
@jayconrod jayconrod added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 13, 2020
@jayconrod jayconrod modified the milestones: Backlog, Unplanned Aug 13, 2020
@jayconrod
Copy link
Contributor

Glad to hear that works for you. I'll leave the issue open. Updated the title to clarify this is not specific to -buildmode=c-archive.

@qingyunha
Copy link
Contributor

func GoFilesPackage(gofiles []string) *Package {
ModInit()
for _, f := range gofiles {
if !strings.HasSuffix(f, ".go") {
pkg := new(Package)
pkg.Internal.Local = true
pkg.Internal.CmdlineFiles = true
pkg.Name = f
pkg.Error = &PackageError{
Err: fmt.Errorf("named files must be .go files: %s", pkg.Name),
}
return pkg
}
}
var stk ImportStack
ctxt := cfg.BuildContext

It seems to remove this check will solve the problem. And the buildContext will check the file valid. The function also use by go run.

@nathan-fiscaletti
Copy link
Contributor Author

nathan-fiscaletti commented Aug 16, 2020

Does this patch seem reasonable? Or perhaps it would make more sense if this is only done for specific build types (c-shared or c-archive)

https://github.com/nathan-fiscaletti/go/blob/master/src/cmd/go/internal/load/pkg.go#L2284-L2315

The reasoning behind the chosen file extensions is taken from this paragraph of the cgo documentation:

When the Go tool sees that one or more Go files use the special import "C", it will look for other non-Go files in the directory and compile them as part of the Go package. Any .c, .s, .S or .sx files will be compiled with the C compiler. Any .cc, .cpp, or .cxx files will be compiled with the C++ compiler. Any .f, .F, .for or .f90 files will be compiled with the fortran compiler.

@nathan-fiscaletti
Copy link
Contributor Author

nathan-fiscaletti commented Aug 16, 2020

After trying to build the source I modified, I found that there is a test specifically for this.

Related commit: a16dcc0

@nathan-fiscaletti
Copy link
Contributor Author

I've modified the patch, should I just submit this as a PR?

master...nathan-fiscaletti:master

@gopherbot
Copy link

Change https://golang.org/cl/248685 mentions this issue: cmd/go: support non .go named files in go build

@jayconrod
Copy link
Contributor

After thinking about this a bit more and reading through https://golang.org/cl/248685, I don't think we should make this change.

There's potential for several bugs and breaking changes to be introduced, and I don't think that changing behavior delivers enough value to be worth the extra complication.

  • There's always been ambiguity about whether an argument to go build is a file name or a package path. Currently, we say that if it ends with .go, it's a package path. That's pretty safe, since there's no .go TLD and it's rare for directories to end with .go. Even though this behavior pretty narrow, we have had problems with conflicts with this in the past, and I don't look forward to resolving several more debates like that. This change would significantly expand the set of suffixes that would be considered filenames, resulting in more ambiguity.
  • There must always be at least one .go file in a package. If someone just specifies a package as a list of .c files, that results in a confusing experience.

@nathan-fiscaletti Could you explain why this is needed? From the comments above, it sounded like you could build your package with a regular package path rather than a list of files.

cc @matloob @bcmills

@nathan-fiscaletti
Copy link
Contributor Author

While the method you gave above works perfectly, I feel it's not very intuitive. When I first read about the build constraints, I had incorrectly assumed that they were restricted to Go files and would not work with other files (such as C files) as it didn't explicitly state otherwise.

Additionally, I think there is some inconsistency that's brought about due to the fact that calling go build will include C files when used without named files but I'm unable to manually include them if I'm using named files. The reason for this isn't made clear. If I can compile the C files that are required by my project when not naming any files, what is stopping me from being able to name the C files and have them compiled?

Overall, I think there's a lack of clarity in the documentation around why this restriction exists and, at least for me, wound up confusing me more than anything. So, while I don't necessarily think that the feature needs to be added to the toolchain, I do think that some clarification in the relevant documentation would go a long way.

@jayconrod
Copy link
Contributor

While the method you gave above works perfectly, I feel it's not very intuitive. When I first read about the build constraints, I had incorrectly assumed that they were restricted to Go files and would not work with other files (such as C files) as it didn't explicitly state otherwise.

Hmm, I agree the documentation on Build constraints should be clarified. It doesn't explicitly mention any kind of file, so it's easy to assume they only work in .go files.

Additionally, I think there is some inconsistency that's brought about due to the fact that calling go build will include C files when used without named files but I'm unable to manually include them if I'm using named files. The reason for this isn't made clear. If I can compile the C files that are required by my project when not naming any files, what is stopping me from being able to name the C files and have them compiled?

There's no technical reason this isn't possible. We're mainly optimizing the command line interface for regular packages, and I think allowing other kinds of files would lead to more confusing edge cases for the reasons outlined above. Packages specified as individual files can't be imported, so they tend to be small ad hoc programs and examples: a less common use case, especially when cgo is mixed in.

@nathan-fiscaletti
Copy link
Contributor Author

especially when cgo is mixed in

Perhaps my viewpoint is biased as I almost exclusively work with CGo.

Is there a way to submit a PR that updates the related documentation? I'm still fairly new to contributing to Go, but I'd be happy to update the relevant documentation.

@jayconrod
Copy link
Contributor

Sure, if you're interested in updating this, the page I linked is in src/cmd/go/internal/help/helpdoc.go. Make sure to run mkalldocs.sh in src/cmd/go to ensure the package documentation is in sync with the go help buildconstraint output.

@nathan-fiscaletti
Copy link
Contributor Author

nathan-fiscaletti commented Aug 20, 2020

Hmm, I agree the documentation on Build constraints should be clarified. It doesn't explicitly mention any kind of file, so it's easy to assume they only work in .go files.

I guess I was incorrect. The first line of the Build Constraints documentation states:

Constraints may appear in any kind of source file (not just Go), but they must appear near the top of the file, preceded only by blank lines and other line comments.

That being the case, I updated the go help build documentation in #40935

@gopherbot
Copy link

Change https://golang.org/cl/249657 mentions this issue: cmd/go: updates documentation for go help c``

@seankhliao seankhliao added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Dec 26, 2022
@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@golang golang locked and limited conversation to collaborators Jan 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants