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

UTF-8 encoding on MS-Windows #3292

Closed
carlosmrce opened this Issue Aug 27, 2015 · 8 comments

Comments

Projects
None yet
3 participants
@carlosmrce

carlosmrce commented Aug 27, 2015

Hi there,

I posted an issue a while back, but it seems to be back!!!

Original issue: #1198

Just tested on MS-Windows and Linux, and the bug still exists.

bug

Thanks!

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Aug 27, 2015

Member

My fix seems to have been removed on master (probably during native IO translation work). @headius can you see if this can be re-shoehorned in. I am guessing native path is not being called and non-native should have #1198 re-applied.

Member

enebo commented Aug 27, 2015

My fix seems to have been removed on master (probably during native IO translation work). @headius can you see if this can be re-shoehorned in. I am guessing native path is not being called and non-native should have #1198 re-applied.

@enebo enebo added this to the JRuby 9.0.1.0 milestone Aug 27, 2015

@enebo enebo added the windows label Aug 27, 2015

@enebo enebo modified the milestones: JRuby 9.0.1.0, JRuby 9.0.2.0 Sep 2, 2015

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 3, 2015

Member

Heh:

    // io_fwrite
    public long fwrite(ThreadContext context, IRubyObject str, boolean nosync) {
        // TODO: Windows
//        #ifdef _WIN32
//        if (fptr->mode & FMODE_TTY) {
//            long len = rb_w32_write_console(str, fptr->fd);
//            if (len > 0) return len;
//        }
//        #endif
        str = doWriteconv(context, str);
        ByteList strByteList = ((RubyString)str).getByteList();
        return binwrite(context, str, strByteList.unsafeBytes(), strByteList.begin(), strByteList.length(), nosync);
    }

I will DO the TODO now :-)

Member

headius commented Sep 3, 2015

Heh:

    // io_fwrite
    public long fwrite(ThreadContext context, IRubyObject str, boolean nosync) {
        // TODO: Windows
//        #ifdef _WIN32
//        if (fptr->mode & FMODE_TTY) {
//            long len = rb_w32_write_console(str, fptr->fd);
//            if (len > 0) return len;
//        }
//        #endif
        str = doWriteconv(context, str);
        ByteList strByteList = ((RubyString)str).getByteList();
        return binwrite(context, str, strByteList.unsafeBytes(), strByteList.begin(), strByteList.length(), nosync);
    }

I will DO the TODO now :-)

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 3, 2015

Member

FWIW, most of the divergence between native and non-native is wrapped up inside PosixShim, which has native and non-native paths. That lives at the level of a C library "write" call, for example. The rest of the ported MRI code I tried to keep as close to MRI as possible and abstract those native/non paths elsewhere.

Fix coming. I don't have Windows handy to test, but perhaps we poke the nightly build and give @carlosmrce a build to confirm with, ok?

Member

headius commented Sep 3, 2015

FWIW, most of the divergence between native and non-native is wrapped up inside PosixShim, which has native and non-native paths. That lives at the level of a C library "write" call, for example. The rest of the ported MRI code I tried to keep as close to MRI as possible and abstract those native/non paths elsewhere.

Fix coming. I don't have Windows handy to test, but perhaps we poke the nightly build and give @carlosmrce a build to confirm with, ok?

@headius headius closed this in aae72b5 Sep 3, 2015

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 3, 2015

Member

If anyone has an idea of how to test this, I'd love to hear it :-\

Edit: for example, running as a subprocess would not be a tty, so that doesn't work. Seems like we need a way to read back what was written to the console.

Member

headius commented Sep 3, 2015

If anyone has an idea of how to test this, I'd love to hear it :-\

Edit: for example, running as a subprocess would not be a tty, so that doesn't work. Seems like we need a way to read back what was written to the console.

@carlosmrce

This comment has been minimized.

Show comment
Hide comment
@carlosmrce

carlosmrce Sep 4, 2015

I will be more than happy to test this out @headius

carlosmrce commented Sep 4, 2015

I will be more than happy to test this out @headius

@carlosmrce

This comment has been minimized.

Show comment
Hide comment
@carlosmrce

carlosmrce Sep 5, 2015

Worked perfectly @enebo @headius

Thanks!

carlosmrce commented Sep 5, 2015

Worked perfectly @enebo @headius

Thanks!

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 19, 2015

Member

Note for future: System.console() can return null when stdio is not connected to a console, causing this fix to blow up in subprocesses. I'm adding a null check that will act similar to the TTY check in the MRI code.

Member

headius commented Oct 19, 2015

Note for future: System.console() can return null when stdio is not connected to a console, causing this fix to blow up in subprocesses. I'm adding a null check that will act similar to the TTY check in the MRI code.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 20, 2015

Member

Note for a happier future: we should just go full native on this and hook into any neccesary transcoding. I fixed this originally with System.console() because it was super easy and works mostly.

A second note is that I think our sub-process spawning is done wrong in jnr-posix and if it was done properly System.console() would not be null.

The second note would make many more cases work and the former will work in cases where our default_internal is not a Java Charset.

Member

enebo commented Oct 20, 2015

Note for a happier future: we should just go full native on this and hook into any neccesary transcoding. I fixed this originally with System.console() because it was super easy and works mostly.

A second note is that I think our sub-process spawning is done wrong in jnr-posix and if it was done properly System.console() would not be null.

The second note would make many more cases work and the former will work in cases where our default_internal is not a Java Charset.

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