The state of windows build

[lennart]

As I tried (for hours) to get the project built on windows, I just wanted to ask what the current state of porting it to work on windows is.

After I got taglib 1.8 to compile through MSVC (Express Version 2010), I struggled getting the binding compiled. Some things like sys/time.h need to be #ifdeffed for Windows to include just time.h, your "now()" function seems to be uneeded and doesn't compiled so this went out as well.

In the end there where some weird bugs, concerning "no appropriate default constructor available" on initialization of AsyncBaton.

Maybe I could open up a windows branch and document the errors in the whole…

Compilation was on Windows 7 32Bit as 64bit taglib didn't compile (some linker error) Node 0.8.20 32bit, latest node-gyp

Let me know what you think.

best regards,

Lennart

[nikhilm]

A branch sounds good. Thanks a lot! I really appreciate it.

On Fri, Mar 1, 2013 at 9:26 AM, Lennart Melzer notifications@github.comwrote:

As I tried (for hours) to get the project built on windows, I just wanted to ask what the current state of porting it to work on windows is.

After I got taglib 1.8 to compile through MSVC (Express Version 2010), I struggled getting the binding compiled. Some things like sys/time.h need to be #ifdeffed for Windows to include just time.h, your "now()" function seems to be uneeded and doesn't compiled so this went out as well.

In the end there where some weird bugs, concerning "no appropriate default constructor available" on initialization of AsyncBaton.

Maybe I could open up a windows branch and document the errors in the whole…

Compilation was on Windows 7 32Bit as 64bit taglib didn't compile (some linker error) Node 0.8.20 32bit, latest node-gyp

Let me know what you think.

best regards,

Lennart

— Reply to this email directly or view it on GitHubhttps://github.com/nikhilm/node-taglib/issues/38 .

[lennart]

Ok I added the branch in my fork,

as I said this currently does not fully compile.

I'll add documentation that is windows specific to the README.

With a working compiled taglib in C:/taglib (currently hardcoded, as I couldn't get taglib/config to work), one can successfully compile bufferstream.cc and link successfully against taglib, starting with tag.cc, or taglib.cc it breaks.

I'll need to reconfigure the windows machine to display english error messages, since german may be rather useless for you.

But tell me, does the AsyncBaton need to be a struct? maybe the compilation errors come from the fact that MSVC does not fully implement the C Standard as one might expect and a simple class (not much more than what it does right now) might do it? Suggestions?

For now I can tell you that initializing the AsyncBaton here:

taglib.cc around line 210

    AsyncBaton *baton = new AsyncBaton;

Fails as the compiler says it cannot find a default constructor for AsyncBaton, this seems weird, since it is a struct with no declared constructor, so it should have one. For reference this is MSVC's Error C2512 but it might turn out something completely different causes the problem.

Any ideas? Best regards, I'll add error logs asap.

Lennart

[nikhilm]

How about just adding a

public AsyncBaton() {}

to the struct? I never made it a class because it wasn't required, but whatever works.

[lennart]

Either way this seems to only move the error to TagLib::FileName missing a default constructor (This is really weird, since I guess taglib's code is proven to work on windows)..

Here's the build log

I'll need to dig into the other errors, it might be that the root cause is somewhere else (parallel processing of multiple files being compiled).

Cheers

[lennart]

Hi Nikhil,

I finally got the extension compiled, though async support breaks on runtime. My windows branch has some fixes, that should be non-breaking for master, but I'd like to test some more.

The problem with async seems to be that the baton loses it's stream (your assertion fails), I'll dig into it some more to find out what the problem is.

Sync reading of tags works!

Regards,

Lennart

[lennart]

Ok I found the bug, I just failed to port the code you wrote to the windows equivalent, msbuild complains it cannot compare against baton->path directly, so I added a direct comparison against the TagLib::String::null.toCString(), but however, one should keep the logic path intact (you compared for existence, I had checked for null).

Async support works on windows, haven't run a whole test suite though, but it seems to work.

Best Regards,

Lennart

[nikhilm]

Great job :) I'll review as soon as you open a pull request and I test it. Thanks!

[lennart]

Just a quick feedback after testing the windows build,

it seems, that using "tag" and "tagSync" repeatedly doesn't work, as Windows seem sto have problems opening the file twice if you don't release the created TagLib::FileRef, using "read" more than once works, since it disposes the FileRef on completion.

Is the FileRef needed all the time? Otherwise one could simply create a FileRef on each function call that really needs to access the underlying file, and dispose it afterwards.

On the other hand, I am not sure if this is another bug in the Windows version, that prevents opening a file twice via taglib.

I'll try to compile a simple c++ test, trying the aforementioned initializing two FileRef after another on windows.

Regards,

Lennart

[nikhilm]

The FileRef has to be kept around when a tag object is in use because writing requires it to be around, and we can't get rid of it until the JS object has been GCed.

The alternative is to delete the FileRef immediately, and to re-open it when save(Sync)() is called, as you've said.

I don't know if its a windows bug, but keeping the FileRef's around does cause a problem in Unixes of eventually reaching the per process max open fd limit. So your idea may actually help both things. I can try to hack something this weekend.

Thanks for all your work :+1:

[lennart]

Concerning building taglib on windows, do you think we could ship the extension with prebuilt DLLs for standard windows distributions (e.g. currently I could add a working DLL for Win 7 32bit, as 64bit taglib fails). This way one could minimize the hassle for windows users that comes with building the git version of taglib yourself.

gyp allows you to copy files into the build/Release folder after succeeded compilation, so one could drop in those DLLs. Otherwise as the node-expat bindings do, one could simply add the taglib source itself and compile it as a dependency before compiling the node extension.

What do you think?

[nikhilm]

How large are the DLLs? If they are relatively small, and as long as their are no licensing issues, I'm fine with this.

Option 2 (compiling taglib as a dependency) is definitely preferable as long as it doesn't involve too much hacking to get it working on all platforms, and it only installs taglib from source if I don't have it installed.

[lennart]

tag.dll for windows 7 on 32Bit is roughly 700kb, additionally the zlib DLL is about 150KB

I'll look into compiling taglib from source.

While trying to get your memtag branch working on Windows I discovered the cause for the problems with the FileRefs I guess.

On Windows TagLib::FileName ist not a simple typedef but a whole class which lacks an explicit destructor. MSVC denies delete against these constructs, so I hacked in an explicit cast which seemed to have caused the problems.

If I replace all references to TagLib::FileName with const char* (which is what TagLib::FileName typedefs to on *nix systems) the async-write.js tests work as expected.

I haven't checked whether the problem is solved against master branch using this hack. There has to be a good reason (probably concerning MSVC's handling of chars and wchars) for taglib to explicitly define FileName as a class.

That said I have some patches ready, but not ready for committing, for a kind-of-working windows version. I'll keep you updated.

Regards,

Lennart

[nikhilm]

I'm on vacation for a few days. I'll check this out on Wednesday.

[lennart]

Hi Nikhil,

I just investigated some more on other issues with the windows build:

• currently Unicode chars in filenames will not work with the bindings
• the problem with "no appropriate default constructor" for the Batons persists

Unicode filename handling obviously fails when using my hack (using only const char*) as the v8::String::Utf8Value is corrupted when casted directly to const char* since Unicode symbols seem to get lost.

I am still stuck with the "no appropriate default constructor" problem, since I have no real clue why it fails to find a fitting default constructor for the Baton only on windows.

Just to let you know, I have adapted node-expat's binding.gyp to build zlib & taglib as dependencies of the node binding from source. The compiled binding is roughly usable for async read an write operations (not 100% tested)

Best regards,

Lennart