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

Csound::Compile() won't work if non-English characters in path.. #632

Closed
rorywalsh opened this issue Apr 21, 2016 · 34 comments
Closed

Csound::Compile() won't work if non-English characters in path.. #632

rorywalsh opened this issue Apr 21, 2016 · 34 comments

Comments

@rorywalsh
Copy link
Contributor

Many of my students have non-English characters in their names. Their home directory, which is the base of all their document paths, therefore contains non English characters. When I use these paths with Csound::Compile() I get:

csound->FileOpen2 failed:: No such file or directory

Below is a simple .cpp file you can test with it. Note that I've only tested this on Windows. I think it's working fine on other platforms. There is no problem when I use the offending path directly with the Csound command line.

#include <stdio.h>
#include "csound.hpp"

int main(int argc, char *argv[])
{
Csound* csound = new Csound();

//doesn't work....
csound->Compile("C:/Users/rory/Desktop/Ádhamh/quickTest.csd");

//works fine....
csound->Compile("C:/Users/rory/Desktop/quickTest.csd");

csound->Start();
csound->Perform();  
delete csound;

return 0;
}
@vlazzarini
Copy link
Member

I tried a similar test on OSX

csound->Compile("àèù.csd");

and it passed.

@fsateler
Copy link
Contributor

I think the problem is that the string encoding in the source file does not match the encoding used at runtime, thus "special" characters get garbled. @rorywalsh, does the problem persist if you pass the filename via a command line argument to your test program? Also, you can test changing the file encoding to different things (try with utf-8, notepad++ has a Encoding menu that can be used to easily switch)

@rorywalsh
Copy link
Contributor Author

Thanks Fiipe, passing the filename from the command line does work but encoding my files as utf8 doesn't make any difference. Any other ideas?

@rorywalsh
Copy link
Contributor Author

rorywalsh commented Apr 22, 2016

Csound seems to replace the Á with ├ü on Windows? There's hardly a compiler setting I'm overlooking or something?

csound->FileOpen2 failed:: No such file or directory
UnifiedCSD: C:/Users/rory/Desktop/Ádhamh/quickTest.csd

[edit] Just found this link, https://sourceforge.net/p/mingw-w64/wiki2/Unicode%20apps/, perhaps that's the key. I'll try it out and post back when I get the chance.

@fsateler
Copy link
Contributor

Rory, did you try other encoding options?

Unfortunately, because c strings have 8bit characters, your sources need to
match the system encoding for this to work. More unfortunately, I have no
idea how to determine the system encoding on windows.
On 22 Apr 2016 06:47, "Rory Walsh" notifications@github.com wrote:

Csound seems to replace the Á with ├ü on Windows? There's hardly a
compiler setting I'm overlooking or something?

csound->FileOpen2 failed:: No such file or directory
UnifiedCSD: C:/Users/rory/Desktop/Ádhamh/quickTest.csd


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#632 (comment)

@rorywalsh
Copy link
Contributor Author

I did Felipe, but none seem to work. I'll keep digging.

@rorywalsh
Copy link
Contributor Author

It seems the fopen() on windows at least will not return a valid FILE pointer if non-ascii chars are in the file path. The solution is to use _wfopen() which I think is Windows only. Using _wfopen() instead of fopen in the following code fixes the issue.

https://github.com/csound/csound/blob/develop/Engine/envvar.c#L1011-L1019

But it's obviously Windows only. Would the devs mind implementing this with some if-def blocks?

@vlazzarini
Copy link
Member

that could be done for some files, but I wonder if libsndfile would do this too? Can you try and see
if diskin opens a file with wide characters?

@jpffitch
Copy link
Contributor

If arguents te sane or near same I would prefer a macro so easy to use all
over

On Fri, 22 Apr 2016, Rory Walsh wrote:

It seems the fopen() on windows at least will not return a valid FILE pointer
if non-ascii chars are in the file path. The solution is to use _wfopen()
which I think is Windows only. Using _wfopen() instead of fopen in the
following code fixes the issue.

https://github.com/csound/csound/blob/develop/Engine/envvar.c#L1011-L1019

But it's obviously Windows only. Would the devs mind implementing this with
some if-def blocks?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on
GitHub[AELNQh9NmclfZKPR5vWvCEboKl3wwcPjks5p6N17gaJpZM4IMpxd.gif]

@rorywalsh
Copy link
Contributor Author

rorywalsh commented Apr 22, 2016

@vlazzarini : diskin2 fails with wide characters. In fact, it seems that any strings with wide chars chars on Windows will not be read properly. Include files that use non-ascii paths also return a not found error.

@jpffitch in those lines I quoted, fullName needs to be a wchar_t_, while params needs to be cast as a wchar_t_. Something like this does it(I say like this because I'm not at the machine I tested it on, but that's more or less what I did):

    if (env == NULL) {     
#if defined(WIN32)
      fullName = (char*) name;
      size_t sz = MultiByteToWideChar(CP_UTF8, 0, name, -1, NULL, 0);
      wchar_t *wfname = alloca(sz);
      MultiByteToWideChar(CP_UTF8, 0, name, -1, wfname, sz);

      sz = MultiByteToWideChar(CP_UTF8, 0, param, -1, NULL, 0);
      wchar_t *wmode = alloca(sz);
      MultiByteToWideChar(CP_UTF8, 0, param, -1, wmode, sz);

      if (type == CSFILE_STD) {
         tmp_f = _wfopen(wfname, wmode);
         if (tmp_f == NULL) {
           perror(Str("csound->FileOpen2 failed:"));
           goto err_return;
         }
      }
#else
      fullName = (char*) name;
      if (type == CSFILE_STD) {
        tmp_f = fopen(fullName, (char*) param);
        if (tmp_f == NULL) {
          perror(Str("csound->FileOpen2 failed:"));
          goto err_return;
        }
      }
#endif

@vlazzarini
Copy link
Member

not sure if we call fopen or we let libsndfile do it for us. If it is the second case, then you'll need to ask Erik.

@rorywalsh
Copy link
Contributor Author

Considering Csound can't handle non-ascii characters the diskin issue is not as big one for me. (although it may well confuse some users). Not being able to compile a file with the API that has non-ascii characters in its path concerns me more.

@vlazzarini
Copy link
Member

Ok, I looked and we use libsndfile sf_open etc functions to open soundfile. So that's that. As for standard files, we use fopen, which could be replaced on windows for a more suitable function. It's in envvar.c . You could possibly try it and see if it does anything, then we could look at adding to the sources.

@rorywalsh
Copy link
Contributor Author

The solution is the same as the one I posted a few hours ago, we should use _wfopen().

@vlazzarini
Copy link
Member

That may be, but it would be good if you could try and let us know. We're linux-osx people here.

Victor Lazzarini
Dean of Arts, Celtic Studies, and Philosophy
Maynooth University
Ireland

On 22 Apr 2016, at 20:51, Rory Walsh notifications@github.com wrote:

The solution is the same as the one I posted a few hours ago, we should use _wfopen().


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@rorywalsh
Copy link
Contributor Author

I have tried it. I tested it on Windows just before I posted the solution. That's why I refer to it as a "solution" ;) But I'd still rather leave it to you guys to implement it the way you see fit.

@vlazzarini
Copy link
Member

I see. But we need a Windows developer to implement it all, so we are not doing it blindly.

@rorywalsh
Copy link
Contributor Author

I think this is sorted now. I've updated my original code snippet with working code. I've tested with simple command line API programs and I've also tested it with Cabbage. I left
fullName = (char*) name;
within the if-def block as fullname appears to be used later in that function.

@jpffitch
Copy link
Contributor

When you say sorted does it need changes to csound sources?

@rorywalsh
Copy link
Contributor Author

Yes, if you wouldn't mind. The code is posted above.

On 24 April 2016 at 15:43, John ffitch notifications@github.com wrote:

When you say sorted does it need changes to csound sources?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#632 (comment)

@jpffitch
Copy link
Contributor

In git; did I get it right?
Please check

@rorywalsh
Copy link
Contributor Author

rorywalsh commented Apr 24, 2016

There seems to be a stray bracket there. I've emailed you the fixed version.

On 24 April 2016 at 17:31, John ffitch notifications@github.com wrote:

In git; did I get it right?
Please check


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#632 (comment)

@gogins
Copy link
Contributor

gogins commented Apr 24, 2016

I also found and fixed this.

Regards,
Mike


Michael Gogins
Irreducible Productions
http://michaelgogins.tumblr.com
Michael dot Gogins at gmail dot com

On Sun, Apr 24, 2016 at 2:14 PM, Rory Walsh notifications@github.com
wrote:

There seems to be a stray bracket there. I've attached the fixed version.

On 24 April 2016 at 17:31, John ffitch notifications@github.com wrote:

In git; did I get it right?
Please check


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#632 (comment)


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#632 (comment)

@jpffitch
Copy link
Contributor

jpffitch commented Apr 24, 2016

The code is a partial fix at best. There are other calls to fopen and open in envvar.c that are unmodified
so writing to files differs from reading. We need a comprehensible fix. Some sndfile opens are OK as they use the fd from open.

I still cannot see any extra brackets either

@rorywalsh
Copy link
Contributor Author

I just ran into an issue with this latest change when using Microsoft's .NET framework. It seems that it doesn't appreciate the call to _wfopen() and produces a problem with msvcrt.dll. It's not easy to fix as there are no debug tools that can help. But reverting the code does make the issue go away. I'm not sure what's the best course of action here. The fix allows us to open files paths with non-English characters on Windows, but causes a problem for anyone using Csound with .NET. What would people think about a new compile method to deal with non-English paths? Would this be appropriate? Users could just call this instead of the normal csoundCompile() on Windows if they wanted it? Or has anyone any other suggestions?

@kunstmusik kunstmusik added this to the 6.08 milestone Aug 24, 2016
@jpffitch
Copy link
Contributor

is it possible to detect at runtime that .net is being used?

There are a number of calls to fopen in the sources. Which do you think should get the _wfopen treatment?

@rorywalsh
Copy link
Contributor Author

I'm not sure. I wouldn't suggest any of them get the _wfopen() treatment if it means not being able to use Csound with .net. I don't know how best to proceed.

@gogins
Copy link
Contributor

gogins commented Oct 10, 2016

Provide a "_w" variant.

Regards,
Mike

On Oct 10, 2016 9:52 AM, "Rory Walsh" notifications@github.com wrote:

I'm not sure. I wouldn't suggest any of them get the _wfopen() treatment
if it means not being able to use Csound with .net. I don't know how best
to proceed.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#632 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AGCDJUxsSpu1YxlhN23cQ5m_kgLSjaV6ks5qykMzgaJpZM4IMpxd
.

@jpffitch
Copy link
Contributor

I thought the problem was in .net

Sent from TypeApp

On 10 Oct 2016, 16:35, at 16:35, Michael Gogins notifications@github.com wrote:

Provide a "_w" variant.

Regards,
Mike

On Oct 10, 2016 9:52 AM, "Rory Walsh" notifications@github.com wrote:

I'm not sure. I wouldn't suggest any of them get the _wfopen()
treatment
if it means not being able to use Csound with .net. I don't know how
best
to proceed.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#632 (comment),
or mute
the thread

https://github.com/notifications/unsubscribe-auth/AGCDJUxsSpu1YxlhN23cQ5m_kgLSjaV6ks5qykMzgaJpZM4IMpxd
.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#632 (comment)

@rorywalsh
Copy link
Contributor Author

Here's a quick run down of the issue. Csound won't open files that have
non-English characters on Windows unless we use _wfopen(). This is now
added and working as far as I can tell. But after this change was made I
noticed that _wfopen() causes an issue when using DLLImport from C#. I'm
afraid I don't know enough about C# to be able to suggest a fix. But I can
confirm that removing the call to _wfopen() does fix the issue.

On 10 October 2016 at 19:13, John ffitch notifications@github.com wrote:

I thought the problem was in .net

Sent from TypeApp

On 10 Oct 2016, 16:35, at 16:35, Michael Gogins notifications@github.com
wrote:

Provide a "_w" variant.

Regards,
Mike

On Oct 10, 2016 9:52 AM, "Rory Walsh" notifications@github.com wrote:

I'm not sure. I wouldn't suggest any of them get the _wfopen()
treatment
if it means not being able to use Csound with .net. I don't know how
best
to proceed.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#632 (comment),
or mute
the thread

<https://github.com/notifications/unsubscribe-
auth/AGCDJUxsSpu1YxlhN23cQ5m_kgLSjaV6ks5qykMzgaJpZM4IMpxd>
.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#632 (comment)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#632 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACkLGfazcJdpCmVecQgPsYseh42H572Pks5qyoBigaJpZM4IMpxd
.

@vlazzarini
Copy link
Member

I think this is a won't fix case. Can we have an #ifdef C# or something?

@jpffitch
Copy link
Contributor

More a cannot fix as Windows does not provide a working fopen. For now we should just keep this problem in mind

@rorywalsh
Copy link
Contributor Author

Can we not use some kind of if/def as Victor suggested?

On 24 October 2016 at 11:42, John ffitch notifications@github.com wrote:

More a cannot fix as Windows does not provide a working fopen. For now we
should just keep this problem in mind


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#632 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACkLGZWEycMP1CscKQOccZX0PvHJYw7Nks5q3It9gaJpZM4IMpxd
.

@rorywalsh
Copy link
Contributor Author

This sounds like a reasonable solution.

On 20 October 2016 at 21:08, vlazzarini notifications@github.com wrote:

I think this is a won't fix case. Can we have an #ifdef C# or something?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#632 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACkLGUisHZxpjhQU2xRCxCiO3MRZQ7rDks5q18ongaJpZM4IMpxd
.

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

No branches or pull requests

6 participants