Can't compile on windows
Closed this issue · 13 comments
Windows 7 Pro, 64 bit. mingw-w64. go 1.5.
Compilation fails with error "can't convert -1 to mdb_filehandle_t".
This appears to be caused by an assumption that the mdb_filehandle_t will be an int, which is not the case on Windows.
func (env *Env) FD() (uintptr, error) {
var mf C.mdb_filehandle_t
ret := C.mdb_env_get_fd(env._env, &mf)
err := operrno("mdb_env_get_fd", ret)
if err != nil {
return 0, err
}
if mf == C.mdb_filehandle_t(-1) { <<< error. assumes POSIX file handle.
return 0, errNotOpen
}
return uintptr(mf), nil
}
/** An abstraction for a file handle.
* On POSIX systems file handles are small integers. On Windows
* they're opaque pointers.
*/
#ifdef _WIN32
typedef void *mdb_filehandle_t;
#else
typedef int mdb_filehandle_t;
#endif
Thanks for raising the issue! I don't have a development environment set up on my Windows machine yet. But in the meantime if you are willing to test out my branch, bmatsuo/fix-windows-compilation, I think this can begin to be resolved. Please check it out and run go buildmake) and go testmake test).
It seems like the C function mdb_env_get_fd returns mdb_filehandle_t(-1) (cast by C) in the case being checked, even on Windows. This seems a little strange to me. Naively I would think it could return NULL. Do you have any insight on that, @hobocastle?
go build/go test on master
# github.com/bmatsuo/lmdb-go/lmdb
.\mdb.c: In function 'mdb_strerror':
cc1.exe: warning: function may return address of local variable [-Wreturn-local-
addr]
.\mdb.c:1309:7: note: declared here
char buf[1024], *ptr = buf;
^
# github.com/bmatsuo/lmdb-go/lmdb
.\env.go:106: cannot convert -1 (type int) to type C.mdb_filehandle_t
go build for bmatsuo/fix-windows-compilation is fine (aside from the warnings as above)
go test output for bmatsuo/fix-windows-compilation:
# github.com/bmatsuo/lmdb-go/lmdb
.\mdb.c: In function 'mdb_strerror':
cc1.exe: warning: function may return address of local variable [-Wreturn-local-
addr]
.\mdb.c:1309:7: note: declared here
char buf[1024], *ptr = buf;
^
--- FAIL: TestCursor_Txn (0.00s)
env_test.go:470: tempdir: mkdir \tmp\mdb_test579365127: The system canno
t find the path specified.
--- FAIL: TestCursor_DBI (0.00s)
env_test.go:470: tempdir: mkdir \tmp\mdb_test939117754: The system canno
t find the path specified.
...
Looking at the C code itself suggests you're correct - it's getting its value from either mdb_env_open or INVALID_HANDLE_VALUE (-1)
Thank you for testing and helping debug these issues, @hobocastle.
That C warning looks like it could be serious. It may be worth investigating whether that is fixed in the latest version of the C library, and bringing it up in the OpenLDAP mailing list if it is not.
Other than that, it seems I derped in that env_test.go file and hardcoded use of the /tmp directory. Please try again with the latest commit on that branch which should use an OS specific temporary directory. You can also override the temp directory by setting the TMPDIR environment variable.
Getting there! I'll take a look at the causes of the new errors when I get in from work.
# github.com/bmatsuo/lmdb-go/lmdb
.\mdb.c: In function 'mdb_strerror':
cc1.exe: warning: function may return address of local variable [-Wreturn-local-
addr]
.\mdb.c:1309:7: note: declared here
char buf[1024], *ptr = buf;
^
--- FAIL: TestEnv_SetMaxReader (0.01s)
env_test.go:212: unexpected error: mdb_env_set_maxreaders: The device do
es not recognize the command. (!= MDB_INVALID: File is not an LMDB file)
--- FAIL: TestTest1 (0.00s)
lmdb_test.go:21: Cannot create temporary directory
--- FAIL: TestTxn_Drop (0.03s)
txn_test.go:61: mdb_get: mdb_get: The device does not recognize the comm
and.
--- FAIL: TestTxn_OpenDBI_zero (0.02s)
txn_test.go:248: mdb_dbi_open: mdb_get: The device does not recognize th
e command.
--- FAIL: TestTxn_Commit (0.02s)
txn_test.go:322: mdb_txn_commit: mdb_txn_commit: The device does not rec
ognize the command.
--- FAIL: TestTxn_Renew_noReset (0.02s)
txn_test.go:629: renew: mdb_txn_renew: The device does not recognize the
command.
--- FAIL: TestTxn_Reset_writeTxn (0.02s)
txn_test.go:691: renew: mdb_txn_renew: The device does not recognize the
command.
FAIL
exit status 1
FAIL github.com/bmatsuo/lmdb-go/lmdb 0.968s
Hrm. This seems like a more challenging problem.
It seems that the syscall package uses os specific Errno constants (e.g. syscall.EINVAL is 1<<29 + 40 on Windows and 22 on Unix systems). And the C library uses Unix errno values. Here is an excerpt from the mdb_strerror function.
#ifdef _WIN32
/* These are the C-runtime error codes we use. The comment indicates
* their numeric value, and the Win32 error they would correspond to
* if the error actually came from a Win32 API. A major mess, we should
* have used LMDB-specific error codes for everything.
*/
switch(err) {
case ENOENT: /* 2, FILE_NOT_FOUND */
case EIO: /* 5, ACCESS_DENIED */
case ENOMEM: /* 12, INVALID_ACCESS */
case EACCES: /* 13, INVALID_DATA */
case EBUSY: /* 16, CURRENT_DIRECTORY */
case EINVAL: /* 22, BAD_COMMAND */
case ENOSPC: /* 28, OUT_OF_PAPER */
return strerror(err);
default:
;
}
buf[0] = 0;
FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM |
FORMAT_MESSAGE_IGNORE_INSERTS,
NULL, err, 0, ptr, sizeof(buf), (va_list *)pad);
return ptr;
#else
return strerror(err);
#endif
So the weird error message, "The device does not recognize the command." is actually a side effect of this behavior. I can correct the message fairly easily by using the function mentioned above. But I'm not yet sure what I want to do about the general problem of error handling. The function lmdb.IsErrnoSys function now seems very questionable.
I have pushed a new commit to my branch. The end result is that the existing tests using errno values like syscall.EINVAL should work, and all the error messages from the last update should be resolved.
The change effectively performs the translation from C (above), transforming return codes into the corresponding syscall.Errno values. Though it is a little hacky because it hardcodes C errno numbers instead of using names (not sure how to get around this best right now). I'm also a little disappointed with the solution because it uses build tags to exclude Go source files during compilation on different operating systems.
But, go ahead and give it a try. It seems like we are getting very close to green tests. All the failures (sub one due to a missed usage of /tmp) are asserting about error handling. That should mean that the tests asserting proper usage of the library are passing.
# github.com/bmatsuo/lmdb-go/lmdb
.\error_windows.go:9: operrno redeclared in this block
previous declaration at .\error_unix.go:9
Adding // +build !unix and // +build !windows comments into the files gets mostly-passing tests:
=== RUN TestEnv_Open_notExist
--- FAIL: TestEnv_Open_notExist (0.00s)
env_test.go:67: open: mdb_env_open: The operation completed successfully.
=== RUN TestErrno
--- FAIL: TestErrno (0.00s)
error_test.go:15: errno(syscall.EINVAL) != syscall.EINVAL: &lmdb.OpError{Op:"testop", Errno:0x0}
FAIL
exit status 1
FAIL github.com/bmatsuo/lmdb-go/lmdb 0.432s
Ugh. Apologies about the build tags. Thank you for figuring it out. 😄
The // +build !unix tag isn't technically necessary in error_windows.go because of the filename suffix. I thought that error_unix.go would be omitted on Windows but I guess I was mistaken. Good to know.
I will look into these errors. But I am also going to get my own Windows development environment set up this weekend though. So hopefully we won't have to bounce back and forth more.
Thanks again for all your help.
@hobocastle, I was able to get a kludgey dev environment set up in Windows 10 under cygwin. In this environment I was able to get the remaining tests passing.
Please check the latest commits out and let me know how they work for you. I will open a pull request and work on cleaning up any hacks before merging the changes in.
Looks good. Obviously still has the warning from the C side, etc.
2015/11/15 09:19:14 this is just a test
PASS
ok github.com/bmatsuo/lmdb-go/lmdb 1.400s
Thanks for all the help, @hobocastle. I've decided to include the fix in the 1.3.0 release. As we have discovered the problems were widespread and were not limited to something that I broke in the last release. But I am glad that we were able to get things working in a way that did not require changes to the exported API.
Thanks again.