Skip to content
This repository has been archived by the owner on Feb 26, 2019. It is now read-only.

godep save deletes CGO c dependency files, causes builds to fail #422

Open
karalabe opened this issue Feb 24, 2016 · 11 comments
Open

godep save deletes CGO c dependency files, causes builds to fail #422

karalabe opened this issue Feb 24, 2016 · 11 comments

Comments

@karalabe
Copy link

This is something I just noticed now when updating godep, don't know when this regression was introduced. When doing a godep save ./..., the dependent packages get correctly gathered, but if they contain CGO snippets including subfolders of the package, those subfolders do not get included in the godep _workspace (existing ones are deleted), causing builds to fail with missing files.

@freeformz
Copy link

Probably v54 as I made the godep save exact dependencies. The current resolver doesn't know about figuring out / including C dependencies. It accidentally (or on purpose depending on how you look at it) worked before hand, but you had to drop your C deps in the correct spot for it to work.

Can you link me to the repo this is an issue with and/or detail your directory layout?

@twmb
Copy link

twmb commented Mar 1, 2016

This fails on sqlite3: https://github.com/mattn/go-sqlite3

$GOPATH/src/github.com/
                     ├─mattn/go-sqlite3
                     └─twmb/fail/fail.go

fail.go:

package main

import _ "github.com/mattn/go-sqlite3"

func main() {
    println("failure")
}

Failure:

$ godep save ./...
$ go build
# github.com/twmb/fail/vendor/github.com/mattn/go-sqlite3
In file included from vendor/github.com/mattn/go-sqlite3/backup.go:9:
./sqlite3-binding.h:2:10: fatal error: 'code/sqlite3-binding.h' file not found
#include "code/sqlite3-binding.h"
         ^
1 error generated.

godep save does not include the go-sqlite3/code/ directory nor contents:

$ find vendor -type f
vendor/github.com/mattn/go-sqlite3/.gitignore
vendor/github.com/mattn/go-sqlite3/.travis.yml
vendor/github.com/mattn/go-sqlite3/backup.go
vendor/github.com/mattn/go-sqlite3/callback.go
vendor/github.com/mattn/go-sqlite3/doc.go
vendor/github.com/mattn/go-sqlite3/error.go
vendor/github.com/mattn/go-sqlite3/LICENSE
vendor/github.com/mattn/go-sqlite3/README.md
vendor/github.com/mattn/go-sqlite3/sqlite3-binding.c
vendor/github.com/mattn/go-sqlite3/sqlite3-binding.h
vendor/github.com/mattn/go-sqlite3/sqlite3.go
vendor/github.com/mattn/go-sqlite3/sqlite3_icu.go
vendor/github.com/mattn/go-sqlite3/sqlite3_libsqlite3.go
vendor/github.com/mattn/go-sqlite3/sqlite3_load_extension.go
vendor/github.com/mattn/go-sqlite3/sqlite3_omit_load_extension.go
vendor/github.com/mattn/go-sqlite3/sqlite3_other.go
vendor/github.com/mattn/go-sqlite3/sqlite3_windows.go

in go-sqlite3:

$ find code -type f
code/sqlite3-binding.c
code/sqlite3-binding.h
code/sqlite3ext.h

@twmb
Copy link

twmb commented Mar 1, 2016

This can be simplified:

$GOPATH/src/github.com/twmb/
                          ├─fail/fail.go
                          ├─cimports/c/cimport.h
                          └─cimport/cimport.go

fail.go:

package main

import "github.com/twmb/cimport"

func main() {
    println(cimport.Var())
}

cimport.go:

package cimport

/*
#import "c/cimport.h"
*/
import "C"

func Var() int {
    return C.variable
}

cimport.h:

const int variable = 1;

In directory cimport, git init; git add .; git commit -m "minimal cgo"

In directory fail, godep save ./...; go build:

# github.com/twmb/fail/vendor/github.com/twmb/cimport
vendor/github.com/twmb/cimport/cimport.go:4:9: fatal error: 'c/cimport.h' file not found
#import "c/cimport.h"
        ^
1 error generated.

@freeformz
Copy link

The package analyzer doesn't understand parsing cgo imports. The workaround that should work for now is to put the c code in the same directory as the package that uses it. I'm not sure if we'll ever support anything beyond that going forward either. Need to think more about it and look at more projects using cgo before making a decision.

@twmb
Copy link

twmb commented Mar 2, 2016

This definitely wont make things easy but ...

https://github.com/search?utf8=%E2%9C%93&q=include+import+%22C%22+language%3AGo+extension%3A.go&type=Code

It appears most uses are either with #import <lib> or #import <localdir.h>, which wont be broken.

I think this would be easier were it parsed with the go/ast as opposed to regex, but I think regexing the prior enclosing /* */ and including all directories that have an import with quotations would suffice.

@karalabe
Copy link
Author

karalabe commented Mar 2, 2016

@freeformz Sorry I didn't have time sooner to make a repro. However I'm not sure the workaround will be enough for many cases. Usually you have three types of CGO use cases (at least what I've encountered / worked with):

  • Small embedded snippets, usually to access some platform specific stuff.
  • Using C libraries from the host system straight.
  • Using C libraries distributed along the Go codebase.

In the first two scenarios, the C code lives beside the Go code in the same folder/packages, so Godep will handle these cases just fine. The problematic part is the third case, where you want to pull in an entire external library (usually because it's not something so common as to be available on all OSes, or easily installable: e.g. snappy encoding, rocksdb, secp256k1 curve crypto). The issue is that these C dependencies can be quite huge, and are already structured as C projects by themselves, so to use them in your own repo, it's not clean practice to copy in Go files into the C project structure. The solution is to simply have a Go package with the wrappers and a subfolder containing the C code that is being wrapped. And this is exactly the scenario Godep fails on currently.

Maybe a possible solution would be to always copy over .c/.cpp/.h etc files that could be used by CGO? Or perhaps only do this if there's some import "C" statement present that could cause them to be needed? Or perhaps you could also use a flag/env var similar to the vendor experiment flag that would (CGOENABLED) that would alter the godep behavior to pull in the C file types?

@freeformz
Copy link

@twmb godep does use go/parser when analyzing packages, but currently that all get's thrown out and isn't used during the copy process. My next steps are to not throw that data out and preserve it so that it will simplify other code (like copy) and enable things like godep status (and maybe even a godep graph).

@karalabe Thanks. Agreed. I need to dig through some common libraries that use CGO to really understand the assumptions. I don't think just blindly copying over .c/.cpp/.h files anywhere below the package would be sufficient, but maybe it is?

jsha added a commit to letsencrypt/go-safe-browsing-api that referenced this issue Apr 14, 2016
Per tools/godep#422, godep won't find cgo files in
other directories, so vendoring won't work without this fix.
@ghodss
Copy link

ghodss commented Apr 14, 2016

@freeformz Any update on how this might be solved? Workarounds that involve changing the library you're depending on are rather challenging, since those repos are typically out of your control.

@freeformz
Copy link

FWIW sqlite-3 has moved their files around so it will no longer happen with that library after an update of that package: mattn/go-sqlite3@3a55553

@freeformz
Copy link

There may be some stuff in here I can use: https://github.com/golang/tools/blob/master/go/loader/cgo.go

@lucasvo
Copy link

lucasvo commented Feb 10, 2018

Short of asking everyone to reformat their repositories, is there any plan on solving this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants