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

[WIP] Windows #3582

Closed
wants to merge 46 commits into
from

Conversation

@lbguilherme
Contributor

lbguilherme commented Nov 26, 2016

Warning: This is very very initial.

Fixes #26

Roadmap:

  • Make it compile code
  • Add {% if flag?(:windows) %} all over the core library and make the prelude portable
    • Basic IO
    • Fibers
    • Concurrent IO
    • Exceptions (backtraces)
    • Threads
    • Mutex
    • Process
    • Regex (should work out of the box, but needs to compile libpcre)
    • GC
    • Encodings
    • ...
  • Port rest of std library
    • DNS
    • Sockets
    • FileUtils and Dir
    • ...
  • Remove dependency on wine clang.exe hack
  • Compile crystal.exe and self-host it
  • Pass all specs
  • Figure out how to do CI testing for Windows
  • Add support for 32-bit Windows as well
  • Release Crystal for Windows!

How to use it: (from Linux)

  1. Get the branch win from this fork:

    git clone https://github.com/lbguilherme/crystal.git
    git checkout win
    
  2. Install Wine: https://www.winehq.org/

  3. Download Clang 3.8.1 for Windows x64. Install it somewhere with Wine. Make sure this works:

    $ wine bin/clang.exe --version
    clang version 3.8.1 (branches/release_38)
    Target: x86_64-pc-windows-msvc
    
  4. Install MXE from my fork (I'll get this patch upstream at some point mxe/mxe#1539)

    git clone https://github.com/lbguilherme/mxe.git
    git checkout updated-gc
    # Check http://mxe.cc/#requirements for a list of dependencies
    # (or just run the comand below and get an error listing what is missing)
    make MXE_TARGETS=x86_64-w64-mingw32.static gcc gc libevent libiconv # This will take some considerable time
    
  5. Build the crystal compiler having at least LLVM 3.8 in your system. Or maybe download some older clang.exe, compactible with the LLVM in your Crystal. If needed, build it: make

  6. Write some simple Crystal:

    # foo.cr
    ch = Channel(String).new
    
    spawn do
      3.times { puts ch.receive }
    end
    
    ch.send "Hello Crystal"
    ch.send "  running on Windows!"
    sleep 1
    ch.send "\nYEAH!!"
    
  7. And finally: Build code!

./bin/crystal build foo.cr --ll --single-module --cross-compile --target x86_64-windows-gnu --prelude=windows-prelude
wine /clang-path/bin/clang.exe -g3 foo.ll -c -o foo.o
/mxe-path/usr/bin/x86_64-w64-mingw32.static-gcc -g3 foo.o -o foo.exe -liconv -lkernel32 -lgc -levent -lws2_32
  1. And run:

    $ wine foo.exe
    Hello Windows!

FAQ

  • When will it be ready? I'm doing it in my free time, so I really have no idea.

  • Why not use CYGWIN? I really believe Windows should be a first-class target and that this is fundamental for Crystal's future as a language. The more native the better. I would ditch even glibc if it were possible (at some point it will be).

  • How can I help? Pick a very small Crystal code. Try to compile it. See a lot of errors and crashes. Do whatever it takes to make them go away. Send a patch with your changes.

lbguilherme added some commits Nov 26, 2016

Make it able to compile targeting Windows
(almost all stdlib doesn't work right now)
@asterite

This comment has been minimized.

Show comment
Hide comment
@asterite

asterite Nov 26, 2016

Contributor

Nice!

I think we should try, for one release, to push this forward until we have at least a bootstrappable compiler to work with. Then things should be much easier.

I noticed some files are completely hidden behind a {% if flag?(:windows) %}. Maybe we can split these files into three. For example: termios.cr, termios_posix.cr, termios_windows.cr and then termios.cr would do the switch over the flag.

Another thing probably worth doing (on my side) is to mark a whole lib with a CallConvention, so all funs inside it will inherit that call convention. It looks like all windows functions use the X86_StdCall convention, right?

Contributor

asterite commented Nov 26, 2016

Nice!

I think we should try, for one release, to push this forward until we have at least a bootstrappable compiler to work with. Then things should be much easier.

I noticed some files are completely hidden behind a {% if flag?(:windows) %}. Maybe we can split these files into three. For example: termios.cr, termios_posix.cr, termios_windows.cr and then termios.cr would do the switch over the flag.

Another thing probably worth doing (on my side) is to mark a whole lib with a CallConvention, so all funs inside it will inherit that call convention. It looks like all windows functions use the X86_StdCall convention, right?

@lbguilherme

This comment has been minimized.

Show comment
Hide comment
@lbguilherme

lbguilherme Nov 26, 2016

Contributor

I noticed some files are completely hidden behind a {% if flag?(:windows) %}. Maybe we can split these files into three. For example: termios.cr, termios_posix.cr, termios_windows.cr and then termios.cr would do the switch over the flag.

This is similar to what Go does, right? Some files will probably need a full rewrite, like scheduler.cr. Splitting into multiple files is the way to go.

Some compiler support would help here, as it wouldn't be possible anymore to require *. For example: splitting io/file_descriptor.cr into io/file_descriptor.posix.cr and io/file_descriptor.windows.cr fails because io.cr has require "./io/*" in it.

Another thing probably worth doing (on my side) is to mark a whole lib with a CallConvention, so all funs inside it will inherit that call convention. It looks like all windows functions use the X86_StdCall convention, right?

That would be awesome! (That's correct, all WinAPI follows this convention)


Contributor

lbguilherme commented Nov 26, 2016

I noticed some files are completely hidden behind a {% if flag?(:windows) %}. Maybe we can split these files into three. For example: termios.cr, termios_posix.cr, termios_windows.cr and then termios.cr would do the switch over the flag.

This is similar to what Go does, right? Some files will probably need a full rewrite, like scheduler.cr. Splitting into multiple files is the way to go.

Some compiler support would help here, as it wouldn't be possible anymore to require *. For example: splitting io/file_descriptor.cr into io/file_descriptor.posix.cr and io/file_descriptor.windows.cr fails because io.cr has require "./io/*" in it.

Another thing probably worth doing (on my side) is to mark a whole lib with a CallConvention, so all funs inside it will inherit that call convention. It looks like all windows functions use the X86_StdCall convention, right?

That would be awesome! (That's correct, all WinAPI follows this convention)


@asterite

This comment has been minimized.

Show comment
Hide comment
@asterite

asterite Nov 26, 2016

Contributor

Oh, the thing about require is true. I guess for now we'll have to break those into individual requires. I think there aren't many cases like that, maybe io or socket are affected, but there aren't many files under those directories. We could try to let the compiler know how to handle this, but I don't know how would that work... maybe check if other filenames exist and see if, splitting by dots, choose the one for which all of the flags meet.

Contributor

asterite commented Nov 26, 2016

Oh, the thing about require is true. I guess for now we'll have to break those into individual requires. I think there aren't many cases like that, maybe io or socket are affected, but there aren't many files under those directories. We could try to let the compiler know how to handle this, but I don't know how would that work... maybe check if other filenames exist and see if, splitting by dots, choose the one for which all of the flags meet.

@RX14

This comment has been minimized.

Show comment
Hide comment
@RX14

RX14 Nov 26, 2016

Member

How about being able to mark a file as ignored or not at the top of a file, similar to surrounding the whole file in a macro if flags, then simply require every platform's files? Maybe make it so any macro expression works.

Member

RX14 commented Nov 26, 2016

How about being able to mark a file as ignored or not at the top of a file, similar to surrounding the whole file in a macro if flags, then simply require every platform's files? Maybe make it so any macro expression works.

@asterite

This comment has been minimized.

Show comment
Hide comment
@asterite

asterite Nov 26, 2016

Contributor

@RX14 That's a good idea. When I read the {% if flag? %} at the top I thought maybe we could have return inside a macro, something like:

# file_windows.cr
{% return unless flag?(:windows) %}

# ...

So code below the {% return ... %} will only be analyzed if the condition is met. The "problem" is that this would be a special rule in the macro language, but maybe it's not that bad.

Contributor

asterite commented Nov 26, 2016

@RX14 That's a good idea. When I read the {% if flag? %} at the top I thought maybe we could have return inside a macro, something like:

# file_windows.cr
{% return unless flag?(:windows) %}

# ...

So code below the {% return ... %} will only be analyzed if the condition is met. The "problem" is that this would be a special rule in the macro language, but maybe it's not that bad.

@RX14

This comment has been minimized.

Show comment
Hide comment
@RX14

RX14 Nov 26, 2016

Member

@asterite Honestly I wouldn't use return, instead something like ignore_file. And only allow it as the first statement in a file, otherwise it'll get messy.

Member

RX14 commented Nov 26, 2016

@asterite Honestly I wouldn't use return, instead something like ignore_file. And only allow it as the first statement in a file, otherwise it'll get messy.

Show outdated Hide outdated src/fiber.cr
sleep(0)
{% else %}

This comment has been minimized.

@Sija

Sija Nov 26, 2016

Contributor

stray else

@Sija

Sija Nov 26, 2016

Contributor

stray else

This comment has been minimized.

@lbguilherme

lbguilherme Nov 26, 2016

Contributor

Oops, messed up selecting which lines to commit. Fixed.

@lbguilherme

lbguilherme Nov 26, 2016

Contributor

Oops, messed up selecting which lines to commit. Fixed.

@ysbaddaden

This comment has been minimized.

Show comment
Hide comment
@ysbaddaden

ysbaddaden Nov 27, 2016

Member

Eventually, maybe not fully duplicate implementations, but extract the OS specific calls, and limit full extracts for cases where POSIX and Windows have strictly nothing in common.

Some methods may require a specific implementation, but IO:: Buffered#buffered_close, for example, only differs on a single platform specific call. We could keep a single implementation that would call a #close_os_impl method for example.

Same for Scheduler: the overall implementation remains identical, only some helpers are POSIX specific, and some others will be Windows specific.

Member

ysbaddaden commented Nov 27, 2016

Eventually, maybe not fully duplicate implementations, but extract the OS specific calls, and limit full extracts for cases where POSIX and Windows have strictly nothing in common.

Some methods may require a specific implementation, but IO:: Buffered#buffered_close, for example, only differs on a single platform specific call. We could keep a single implementation that would call a #close_os_impl method for example.

Same for Scheduler: the overall implementation remains identical, only some helpers are POSIX specific, and some others will be Windows specific.

@shayneoAtNorwood

This comment has been minimized.

Show comment
Hide comment
@shayneoAtNorwood

shayneoAtNorwood Nov 30, 2016

I know this isn't a useful comment, but I just want to chip in and say this is wonderful stuff guys. Windows support=closer to world domination. mwahaha

I know this isn't a useful comment, but I just want to chip in and say this is wonderful stuff guys. Windows support=closer to world domination. mwahaha

@jsaak

This comment has been minimized.

Show comment
Hide comment
@jsaak

jsaak Dec 4, 2016

You may be familiar with libuv. I do not know why, but crystal uses different library. Maybe you can use libuv in windows port?

jsaak commented Dec 4, 2016

You may be familiar with libuv. I do not know why, but crystal uses different library. Maybe you can use libuv in windows port?

@lbguilherme

This comment has been minimized.

Show comment
Hide comment
@lbguilherme

lbguilherme Dec 4, 2016

Contributor
Contributor

lbguilherme commented Dec 4, 2016

@canyonblue77

This comment has been minimized.

Show comment
Hide comment
@canyonblue77

canyonblue77 Dec 22, 2016

Novice Coder and wanted some feedback. Since this thread is about Windows and there is a 1% chance I could benefit the effort I wanted to share my attempt at porting code for starting a new process from kernel32/Windows library.(linux fork?) No idea what I'm actually doing but hey, fake it till ya make it... Is this close too accurate? Would additional ports of other functions be beneficial? Or just annoying background noise, ya won't hurt my feelings. :)

@[CallConvention("X86_StdCall")]
fun winfork = CreateProcessA(
        lpApplicationName : Char*,
        lpCommandLine : Char*,
        lpProcessAttributes : SECURITY_ATTRIBUTES*,
        lpThreadAttributes : SECURITY_ATTRIBUTES*,
        bInheritHandles : Bool,
        dwCreationFlags : Int,
        lpEnvironment : DWord*,
        lpCurrentDirectory : Char*,
        lpStartupInfo : STARTUPINFO,
        lpProcessInformation : PROCESS_INFORMATION
) : Bool`
struct SECURITY_ATTRIBUTES
        length : Dword
        security_descriptors : Void*
        inherit_handle : Bool
end
struct STARTUPINFO
        cb : Dword
        Reserved : Char*
        Desktop : Char*
        Title : Char*
        dwX : Dword
        dwY : Dword
        dwXSize : Dword
        dwYSize : Dword
        dwXCountChars : Dword
        dwYCountChars : Dword
        dwFillAttribute : Dword
        dwFlags : Dword
        ShowWindow : UInt16
        cbReserved2 : UInt16
        Reserved2 : Void*
        StdInput : Handle
        StdOutput : Handle
        StdError : Handle
end
struct PROCESS_INFORMATION
        Process : Handle
        Thread : Handle
        ProcessId : Dword
        ThreadId : Dword
end

canyonblue77 commented Dec 22, 2016

Novice Coder and wanted some feedback. Since this thread is about Windows and there is a 1% chance I could benefit the effort I wanted to share my attempt at porting code for starting a new process from kernel32/Windows library.(linux fork?) No idea what I'm actually doing but hey, fake it till ya make it... Is this close too accurate? Would additional ports of other functions be beneficial? Or just annoying background noise, ya won't hurt my feelings. :)

@[CallConvention("X86_StdCall")]
fun winfork = CreateProcessA(
        lpApplicationName : Char*,
        lpCommandLine : Char*,
        lpProcessAttributes : SECURITY_ATTRIBUTES*,
        lpThreadAttributes : SECURITY_ATTRIBUTES*,
        bInheritHandles : Bool,
        dwCreationFlags : Int,
        lpEnvironment : DWord*,
        lpCurrentDirectory : Char*,
        lpStartupInfo : STARTUPINFO,
        lpProcessInformation : PROCESS_INFORMATION
) : Bool`
struct SECURITY_ATTRIBUTES
        length : Dword
        security_descriptors : Void*
        inherit_handle : Bool
end
struct STARTUPINFO
        cb : Dword
        Reserved : Char*
        Desktop : Char*
        Title : Char*
        dwX : Dword
        dwY : Dword
        dwXSize : Dword
        dwYSize : Dword
        dwXCountChars : Dword
        dwYCountChars : Dword
        dwFillAttribute : Dword
        dwFlags : Dword
        ShowWindow : UInt16
        cbReserved2 : UInt16
        Reserved2 : Void*
        StdInput : Handle
        StdOutput : Handle
        StdError : Handle
end
struct PROCESS_INFORMATION
        Process : Handle
        Thread : Handle
        ProcessId : Dword
        ThreadId : Dword
end

bcardiff added some commits Dec 29, 2016

Merge crystal v0.20.3
Conflicts:
	src/fiber.cr
	src/io/file_descriptor.cr
@bcardiff

This comment has been minimized.

Show comment
Hide comment
@bcardiff

bcardiff Dec 29, 2016

Member

@lbguilherme which version of wine are you using here?

Sorry about that push I messed with the remote name (I didn't expect github will allow me to push on you fork :-S). I wanted to bring the to the latest crystal version and see if there is anything I can contribute with.

Member

bcardiff commented Dec 29, 2016

@lbguilherme which version of wine are you using here?

Sorry about that push I messed with the remote name (I didn't expect github will allow me to push on you fork :-S). I wanted to bring the to the latest crystal version and see if there is anything I can contribute with.

@lbguilherme

This comment has been minimized.

Show comment
Hide comment
@lbguilherme

lbguilherme Dec 29, 2016

Contributor

@bcardiff I didn't even know it was possible to push there, cool! Thanks for the commits, I'll merge it with current master when I get some free time, this weekend I hope.

I'm always testing with the latest wine only, which as of now is 2.0-rc3

@canyonblue77 Thanks for this! It will be useful when porting Process

Contributor

lbguilherme commented Dec 29, 2016

@bcardiff I didn't even know it was possible to push there, cool! Thanks for the commits, I'll merge it with current master when I get some free time, this weekend I hope.

I'm always testing with the latest wine only, which as of now is 2.0-rc3

@canyonblue77 Thanks for this! It will be useful when porting Process

@david50407

This comment has been minimized.

Show comment
Hide comment
@david50407

david50407 Dec 30, 2016

Contributor

@bcardiff Maybe you can create a pull request to @lbguilherme 's own repo & branch if you have done the merging works. (yes, no rebase for here, but merge commit (PR) still works)

Contributor

david50407 commented Dec 30, 2016

@bcardiff Maybe you can create a pull request to @lbguilherme 's own repo & branch if you have done the merging works. (yes, no rebase for here, but merge commit (PR) still works)

@bcardiff

This comment has been minimized.

Show comment
Hide comment
@bcardiff

bcardiff Dec 30, 2016

Member

I agree with that. I was expecting to do it that way, but i push to the wrong remote and github allowed me to push straight to his branch. That is wierd. I don't expect original owners repo to be able to push into forks.

Definitely PR to his win branch is the way to organize this, hence my apologies before.

Member

bcardiff commented Dec 30, 2016

I agree with that. I was expecting to do it that way, but i push to the wrong remote and github allowed me to push straight to his branch. That is wierd. I don't expect original owners repo to be able to push into forks.

Definitely PR to his win branch is the way to organize this, hence my apologies before.

@david50407

This comment has been minimized.

Show comment
Hide comment
@david50407

david50407 Dec 30, 2016

Contributor

@bcardiff you can modify his branch (PR) because you are a reviewer (or repo/organizaton member) in this PR. That's GitHub's new feature which causes more flexible with PRs.

Contributor

david50407 commented Dec 30, 2016

@bcardiff you can modify his branch (PR) because you are a reviewer (or repo/organizaton member) in this PR. That's GitHub's new feature which causes more flexible with PRs.

@bcardiff

This comment has been minimized.

Show comment
Hide comment
@bcardiff

bcardiff Dec 30, 2016

Member

@lbguilherme I'm still working on getting this in some other environment.

The tip for wine 2.0-rc3 was important. Otherwise I was getting the

$ wine /path/to/clang.exe -g3 foo.ll -c -o foo.o
...
wine: Call from 0x7f53b683ae38 to unimplemented function KERNEL32.dll.GetFinalPathNameByHandleW, aborting

Since you are downloading LLVM-3.9 for windows, that means that when you bin/crystal in linux for cross compiling, you have compiled a linux crystal compiler before right? Because the released crystal compiler for linux uses LLVM-3.5 and the IR code generated from the released compiler is incompatible with 3.9 due to changes in 3.7.

$ wine ~/.wine/drive_c/LLVM/bin/clang.exe -g3 foo.ll -c -o foo.o
foo.ll:274:17: error: expected comma after load's type
  %2 = load i1* @"AtExitHandlers::running:init"
                ^
1 error generated.

So,

  1. have you built a crystal compiler before running bin/crystal?
  2. if so, which linux+llvm versions are you using?
Member

bcardiff commented Dec 30, 2016

@lbguilherme I'm still working on getting this in some other environment.

The tip for wine 2.0-rc3 was important. Otherwise I was getting the

$ wine /path/to/clang.exe -g3 foo.ll -c -o foo.o
...
wine: Call from 0x7f53b683ae38 to unimplemented function KERNEL32.dll.GetFinalPathNameByHandleW, aborting

Since you are downloading LLVM-3.9 for windows, that means that when you bin/crystal in linux for cross compiling, you have compiled a linux crystal compiler before right? Because the released crystal compiler for linux uses LLVM-3.5 and the IR code generated from the released compiler is incompatible with 3.9 due to changes in 3.7.

$ wine ~/.wine/drive_c/LLVM/bin/clang.exe -g3 foo.ll -c -o foo.o
foo.ll:274:17: error: expected comma after load's type
  %2 = load i1* @"AtExitHandlers::running:init"
                ^
1 error generated.

So,

  1. have you built a crystal compiler before running bin/crystal?
  2. if so, which linux+llvm versions are you using?
fix src to be able to build the compiler (in linux)
restore content of prelude to be able to compile the compiler in linux with windows target.

stub regex, iconv for windows

add reference to io/memory due to path io/* expansion and migration to crystal 0.20.3

add (temporal) windows-prelude.cr for easy shrink of samples in windows (require --prelude=empty).

```
# file ./foo.cr
require "./windows-prelude" 
puts "Hello Windows!" 
```

```
$ make # to build the compiler with windows target
$ ./bin/crystal build foo.cr --ll --single-module --cross-compile --target x86_64-windows-gnu --prelude=empty
$ wine /path/to/LLVM-3.9.0/bin/clang.exe  -g3 foo.ll -c -o foo.o
$ /path/to/mxe/usr/bin/x86_64-w64-mingw32.static-gcc -g3 foo.o -o foo.exe -liconv -lkernel32 -lgc -levent -lws2_32
$ wine foo.exe 
Hello Windows!
```

lbguilherme added some commits Dec 31, 2016

Merge pull request #1 from bcardiff/win
fix src to be able to build the compiler (in linux)
@txe

This comment has been minimized.

Show comment
Hide comment
@txe

txe Jun 8, 2017

Contributor

Maybe it's not a good place to ask, sorry then.
I've got the following error and can't figure out how to fix it
@proc_info is initialized in initialize method as you can see it in process.windows.cr

in /mnt/f/github/2/crystal/src/compiler/crystal/codegen/link.cr:96: instantiating 'Process:Class#run(String, Tuple(String))'

              has_pkg_config = Process.run("which", {"pkg-config"}, output: false).success?
                                       ^~~

in /mnt/f/github/2/crystal/src/process.windows.cr:155: instantiating 'new(String, Tuple(String), Nil, Bool, Bool, Bool, Bool, Bool, Nil)'

    status = new(command, args, env, clear_env, shell, input, output, error, chdir).wait
             ^~~

instance variable '@proc_info' of Process must be LibWindows::Process_Information, not Nil

Error: instance variable '@proc_info' was used before it was initialized in one of the 'initialize' methods, rendering it nilable
Contributor

txe commented Jun 8, 2017

Maybe it's not a good place to ask, sorry then.
I've got the following error and can't figure out how to fix it
@proc_info is initialized in initialize method as you can see it in process.windows.cr

in /mnt/f/github/2/crystal/src/compiler/crystal/codegen/link.cr:96: instantiating 'Process:Class#run(String, Tuple(String))'

              has_pkg_config = Process.run("which", {"pkg-config"}, output: false).success?
                                       ^~~

in /mnt/f/github/2/crystal/src/process.windows.cr:155: instantiating 'new(String, Tuple(String), Nil, Bool, Bool, Bool, Bool, Bool, Nil)'

    status = new(command, args, env, clear_env, shell, input, output, error, chdir).wait
             ^~~

instance variable '@proc_info' of Process must be LibWindows::Process_Information, not Nil

Error: instance variable '@proc_info' was used before it was initialized in one of the 'initialize' methods, rendering it nilable
@asterite

This comment has been minimized.

Show comment
Hide comment
@asterite

asterite Jun 11, 2017

Contributor

@txe In create_child_process you are invoking close, which uses @proc_info. Better declare that as a nilable type (as it should be, because you have many if @proc_info checks, so...)

Contributor

asterite commented Jun 11, 2017

@txe In create_child_process you are invoking close, which uses @proc_info. Better declare that as a nilable type (as it should be, because you have many if @proc_info checks, so...)

@txe

This comment has been minimized.

Show comment
Hide comment
@txe

txe Jun 11, 2017

Contributor

I see, thanks. I thought of making it nilable, just wanted to find the better solution in terms of strong typing.

Contributor

txe commented Jun 11, 2017

I see, thanks. I thought of making it nilable, just wanted to find the better solution in terms of strong typing.

@txe

This comment has been minimized.

Show comment
Hide comment
@txe

txe Jun 11, 2017

Contributor

I've made module Process more or less working on Windows. There are limitations so far:

  • fork cannot be implemented on Windows, fortunately there is only one place in compiler that depends on fork and that is easy to fix. Can be rewritten when Crystal get multi-threading.
  • reading a process output and waiting for a process exit block a program execution. I suppose, it can be done concurrent, but I tried to keep things simple.

For now I try to compile compiler_spec.cr on Windows. There are problems with LLVM verification of module:

  • Call parameter type does not match function signature
  • Unwind edges out of a catch must have the same unwind dest as the parent catchswitch

I think it takes some time to fix.

Contributor

txe commented Jun 11, 2017

I've made module Process more or less working on Windows. There are limitations so far:

  • fork cannot be implemented on Windows, fortunately there is only one place in compiler that depends on fork and that is easy to fix. Can be rewritten when Crystal get multi-threading.
  • reading a process output and waiting for a process exit block a program execution. I suppose, it can be done concurrent, but I tried to keep things simple.

For now I try to compile compiler_spec.cr on Windows. There are problems with LLVM verification of module:

  • Call parameter type does not match function signature
  • Unwind edges out of a catch must have the same unwind dest as the parent catchswitch

I think it takes some time to fix.

@txe

This comment has been minimized.

Show comment
Hide comment
@txe

txe Jun 12, 2017

Contributor

Took a look at Unwind edges out of a catch must have the same unwind dest as the parent catchswitch. Simple code to repeat the issue:

def test
    "-"
ensure
    File.delete("222.333") rescue nil
end

test
Contributor

txe commented Jun 12, 2017

Took a look at Unwind edges out of a catch must have the same unwind dest as the parent catchswitch. Simple code to repeat the issue:

def test
    "-"
ensure
    File.delete("222.333") rescue nil
end

test
@mjblack

This comment has been minimized.

Show comment
Hide comment
@mjblack

mjblack Jun 15, 2017

Just throwing it out there but to remove the dependency on the wine hack. If you use Windows 10 and enable Windows Subsystem for Linux, you can build crystal both for WSL environment and also have it call clang.exe without wine. The "Creators Update" update for Windows 10 added native support for calling windows exe files from within the WSL environment. This does not solve the CI requirement needed since Travis wont support windows builds except through wine. There are other CI out there that have a free tier for opensource projects that do support windows build environments.

mjblack commented Jun 15, 2017

Just throwing it out there but to remove the dependency on the wine hack. If you use Windows 10 and enable Windows Subsystem for Linux, you can build crystal both for WSL environment and also have it call clang.exe without wine. The "Creators Update" update for Windows 10 added native support for calling windows exe files from within the WSL environment. This does not solve the CI requirement needed since Travis wont support windows builds except through wine. There are other CI out there that have a free tier for opensource projects that do support windows build environments.

@txe

This comment has been minimized.

Show comment
Hide comment
@txe

txe Jun 15, 2017

Contributor

@bcardiff could you give me a hand in fixing windows_runtime_exception_handling method in Crystal::CodeGenVisitor (exception.cr)?
There is an issue when there is a rescue inside another rescue, like

def test
    File.delete("555")
rescue
    begin
        File.delete("222.333")
    rescue
        nil
    end
end

According to LLVM docs, the inner catchswitch should reference to the outer catchpad. I just have no idea how to make that the inner rescue will know about the outer rescue.

Contributor

txe commented Jun 15, 2017

@bcardiff could you give me a hand in fixing windows_runtime_exception_handling method in Crystal::CodeGenVisitor (exception.cr)?
There is an issue when there is a rescue inside another rescue, like

def test
    File.delete("555")
rescue
    begin
        File.delete("222.333")
    rescue
        nil
    end
end

According to LLVM docs, the inner catchswitch should reference to the outer catchpad. I just have no idea how to make that the inner rescue will know about the outer rescue.

@bcardiff

This comment has been minimized.

Show comment
Hide comment
@bcardiff

bcardiff Jun 19, 2017

Member

@txe from looking to the code probably the nil at https://github.com/txe/crystal/blob/win/src/compiler/crystal/codegen/exception.cr#L52 might need to change to @catch_pad. So after it is assigned in https://github.com/txe/crystal/blob/win/src/compiler/crystal/codegen/exception.cr#L120, and the inner rescue body is visited, there reference to the parent catch_pad will be there.

Member

bcardiff commented Jun 19, 2017

@txe from looking to the code probably the nil at https://github.com/txe/crystal/blob/win/src/compiler/crystal/codegen/exception.cr#L52 might need to change to @catch_pad. So after it is assigned in https://github.com/txe/crystal/blob/win/src/compiler/crystal/codegen/exception.cr#L120, and the inner rescue body is visited, there reference to the parent catch_pad will be there.

@oprypin

This comment has been minimized.

Show comment
Hide comment
@oprypin

oprypin Jul 10, 2017

Contributor

I used to perceive this porting work as something really basic and needing a lot of hacks and breaking all the time. And I assume that's how others generally perceive it.

But after going from that mindset to building video games for Windows using Crystal I think it's really time for the core team to take a good look at this work (@txe's branch specifically) and merge it as soon as possible, in order to kickstart the ability for others to incrementally improve Windows support; lower the barrier of entry. I mean, as long it's not breaking anything existing, how can we keep ignoring it?

Contributor

oprypin commented Jul 10, 2017

I used to perceive this porting work as something really basic and needing a lot of hacks and breaking all the time. And I assume that's how others generally perceive it.

But after going from that mindset to building video games for Windows using Crystal I think it's really time for the core team to take a good look at this work (@txe's branch specifically) and merge it as soon as possible, in order to kickstart the ability for others to incrementally improve Windows support; lower the barrier of entry. I mean, as long it's not breaking anything existing, how can we keep ignoring it?

@RX14

This comment has been minimized.

Show comment
Hide comment
@RX14

RX14 Jul 10, 2017

Member

@oprypin because it actually does break existing things by renaming a bunch of files and not using Crystal::System.

Member

RX14 commented Jul 10, 2017

@oprypin because it actually does break existing things by renaming a bunch of files and not using Crystal::System.

@oprypin

This comment has been minimized.

Show comment
Hide comment
@oprypin

oprypin Jul 10, 2017

Contributor

So then... someone familiar with this Crystal::System idea should make a focused effort to fix this up. Or is there some general explanation for what's the right way to do such things?


FWIW, just ran specs on that branch on Linux,

12200 examples, 0 failures, 0 errors, 13 pending

So I would not say this breaks anything

Contributor

oprypin commented Jul 10, 2017

So then... someone familiar with this Crystal::System idea should make a focused effort to fix this up. Or is there some general explanation for what's the right way to do such things?


FWIW, just ran specs on that branch on Linux,

12200 examples, 0 failures, 0 errors, 13 pending

So I would not say this breaks anything

@txe

This comment has been minimized.

Show comment
Hide comment
@txe

txe Jul 10, 2017

Contributor

The branch doesn't contain the latest changes from crystal repository, so this is why there is no failures

Contributor

txe commented Jul 10, 2017

The branch doesn't contain the latest changes from crystal repository, so this is why there is no failures

@bararchy

This comment has been minimized.

Show comment
Hide comment
@bararchy

bararchy Jul 10, 2017

Contributor

@txe How hard would it be to rebase it ?

Contributor

bararchy commented Jul 10, 2017

@txe How hard would it be to rebase it ?

@txe

This comment has been minimized.

Show comment
Hide comment
@txe

txe Jul 10, 2017

Contributor

Even if it's hard, I'm ready to rebase it and add Crystal::System. I just want to know if it will be enough to merge it finally,

Contributor

txe commented Jul 10, 2017

Even if it's hard, I'm ready to rebase it and add Crystal::System. I just want to know if it will be enough to merge it finally,

@mverzilli

This comment has been minimized.

Show comment
Hide comment
@mverzilli

mverzilli Jul 10, 2017

Member

Yes. If it doesn't break tests for already supported platforms we can make it a priority to merge this, aiming at having it in 0.24.

Member

mverzilli commented Jul 10, 2017

Yes. If it doesn't break tests for already supported platforms we can make it a priority to merge this, aiming at having it in 0.24.

@txe

This comment has been minimized.

Show comment
Hide comment
@txe

txe Jul 10, 2017

Contributor

Good, then I will try to prepare the branch this week.

Contributor

txe commented Jul 10, 2017

Good, then I will try to prepare the branch this week.

@RX14

This comment has been minimized.

Show comment
Hide comment
@RX14

RX14 Jul 10, 2017

Member

My concern is that if nobody is willing to port this work over to Crystal::System now why would they be willing to do so after this PR is merged. If that work doesn't happen we're left with an ugly duality of files which have to keep the exact same interface over 2 platforms with nothing enforcing that. Imagine PRing a fix to the posix version of the stdlib which tweaks a method signature a little, what is there to enforce that that change also takes place on windows? That's part of the reason why we have Crystal::System: to create a clean, minimal interface to platform-specifics that changes at a rate much slower than the rest of the standard library.

It also invalidates a lot of the open PRs by renaming the posix-supporting files. Even if this PR doesn't break anything visibly outside the standard library, it does break the flow for everyone already working on crystal by renaming everything platform specific.

Please, let's be patient and do windows support properly.

Member

RX14 commented Jul 10, 2017

My concern is that if nobody is willing to port this work over to Crystal::System now why would they be willing to do so after this PR is merged. If that work doesn't happen we're left with an ugly duality of files which have to keep the exact same interface over 2 platforms with nothing enforcing that. Imagine PRing a fix to the posix version of the stdlib which tweaks a method signature a little, what is there to enforce that that change also takes place on windows? That's part of the reason why we have Crystal::System: to create a clean, minimal interface to platform-specifics that changes at a rate much slower than the rest of the standard library.

It also invalidates a lot of the open PRs by renaming the posix-supporting files. Even if this PR doesn't break anything visibly outside the standard library, it does break the flow for everyone already working on crystal by renaming everything platform specific.

Please, let's be patient and do windows support properly.

@akzhan

This comment has been minimized.

Show comment
Hide comment
@akzhan

akzhan Jul 10, 2017

Contributor

Will CI allowed for Windows builds?

Contributor

akzhan commented Jul 10, 2017

Will CI allowed for Windows builds?

@bararchy

This comment has been minimized.

Show comment
Hide comment
@bararchy

bararchy Jul 10, 2017

Contributor

@RX14 It seems though that @txe is willing to do the System change.

Contributor

bararchy commented Jul 10, 2017

@RX14 It seems though that @txe is willing to do the System change.

@mverzilli

This comment has been minimized.

Show comment
Hide comment
@mverzilli

mverzilli Jul 10, 2017

Member

@RX14, sure. Making this a priority means we should prioritize everything that stands on the way to it as well :). Maybe I rushed a bit to say it was for 0.24, but I don't see it that far.

@akzhan we can start with experimental support (which is to say no-CI). I wouldn't tie this to having a CI on Windows. Otherwise we'll deadlock progress on both.

Member

mverzilli commented Jul 10, 2017

@RX14, sure. Making this a priority means we should prioritize everything that stands on the way to it as well :). Maybe I rushed a bit to say it was for 0.24, but I don't see it that far.

@akzhan we can start with experimental support (which is to say no-CI). I wouldn't tie this to having a CI on Windows. Otherwise we'll deadlock progress on both.

@RX14

This comment has been minimized.

Show comment
Hide comment
@RX14

RX14 Jul 10, 2017

Member

@bararchy I was more responding to @oprypin's suggestion to merge this essentially as-is.

Rebasing this to use Crystal::System is a lot of hard work and design decisions which affect linux as well (but I think it's well worth it). Plus rebasing to use Crystal::System naturally splits up this PR into separately mergeable sections, so at that point you might as well PR each bit seperately to speed up the process.

AFAIK we still have the exception handling issue on windows as well, is that still an issue? That's a compiler change that can be PRed seperately right?

Member

RX14 commented Jul 10, 2017

@bararchy I was more responding to @oprypin's suggestion to merge this essentially as-is.

Rebasing this to use Crystal::System is a lot of hard work and design decisions which affect linux as well (but I think it's well worth it). Plus rebasing to use Crystal::System naturally splits up this PR into separately mergeable sections, so at that point you might as well PR each bit seperately to speed up the process.

AFAIK we still have the exception handling issue on windows as well, is that still an issue? That's a compiler change that can be PRed seperately right?

@txe

This comment has been minimized.

Show comment
Hide comment
@txe

txe Jul 10, 2017

Contributor

As I see, @RX14 is going to create a Crystal::System version of IO::FileDescriptor, so I can wait for it and then use it as an example of Crystal::System to do PR for: process.cr, scheduler.cr, file.cr, file\stat.cr, thread.cr, dir.cr.
What do you think?

Contributor

txe commented Jul 10, 2017

As I see, @RX14 is going to create a Crystal::System version of IO::FileDescriptor, so I can wait for it and then use it as an example of Crystal::System to do PR for: process.cr, scheduler.cr, file.cr, file\stat.cr, thread.cr, dir.cr.
What do you think?

@Qwerp-Derp

This comment has been minimized.

Show comment
Hide comment
@Qwerp-Derp

Qwerp-Derp Jul 29, 2017

Any news on this?

Any news on this?

@RX14

This comment has been minimized.

Show comment
Hide comment
@RX14

RX14 Jul 29, 2017

Member

@Qwerp-Derp The next step is to get #4707 merged.

Member

RX14 commented Jul 29, 2017

@Qwerp-Derp The next step is to get #4707 merged.

@bararchy

This comment has been minimized.

Show comment
Hide comment
@bararchy

bararchy Jul 29, 2017

Contributor

What's holding #4707 back ?

Contributor

bararchy commented Jul 29, 2017

What's holding #4707 back ?

@sdogruyol

This comment has been minimized.

Show comment
Hide comment
@sdogruyol

sdogruyol Aug 30, 2017

Member

Let's rock now that #4707 is merged 👍

Member

sdogruyol commented Aug 30, 2017

Let's rock now that #4707 is merged 👍

@sdogruyol

This comment has been minimized.

Show comment
Hide comment
@sdogruyol

sdogruyol Sep 9, 2017

Member

Update #4832 is also merged 👍

Member

sdogruyol commented Sep 9, 2017

Update #4832 is also merged 👍

@piedoom

This comment has been minimized.

Show comment
Hide comment
@piedoom

piedoom Oct 12, 2017

Is there any news on this? I've been checking here every few weeks to see progress.

piedoom commented Oct 12, 2017

Is there any news on this? I've been checking here every few weeks to see progress.

@Yardanico

This comment has been minimized.

Show comment
Hide comment
@Yardanico

Yardanico Oct 16, 2017

There is AppVeyor for CI testing on Windows

There is AppVeyor for CI testing on Windows

@ylluminate

This comment has been minimized.

Show comment
Hide comment
@ylluminate

ylluminate Oct 27, 2017

Just a heads up, interesting discussion from Nanobox guys with some interest in using Crystal instead of Go (see #crystal) if there was native Windows support. While Windows doesn't personally mean much to me, as a platform and for growing the exposure, Crystal could have some really great wins publicly if this were gnawed on to completion.

Just a heads up, interesting discussion from Nanobox guys with some interest in using Crystal instead of Go (see #crystal) if there was native Windows support. While Windows doesn't personally mean much to me, as a platform and for growing the exposure, Crystal could have some really great wins publicly if this were gnawed on to completion.

@faustinoaq faustinoaq referenced this pull request Nov 14, 2017

Open

Make crystal more beginner friendly #5291

2 of 6 tasks complete
@ilog2000

This comment has been minimized.

Show comment
Hide comment
@ilog2000

ilog2000 Nov 27, 2017

Congratulations on one year anniversary of this pull request!

Congratulations on one year anniversary of this pull request!

@RX14

This comment has been minimized.

Show comment
Hide comment
@RX14

RX14 Nov 29, 2017

Member

If we get #5333 merged, there's a relatively tiny patch (that already works) to getting hello world working on windows. I think that hello world is working as long as it isn't ifdef hell. And after that we can finally get to merging in @txe and @lbguilherme's work. It's been a year but I think we can do it this time.

Member

RX14 commented Nov 29, 2017

If we get #5333 merged, there's a relatively tiny patch (that already works) to getting hello world working on windows. I think that hello world is working as long as it isn't ifdef hell. And after that we can finally get to merging in @txe and @lbguilherme's work. It's been a year but I think we can do it this time.

@RX14 RX14 referenced this pull request Dec 1, 2017

Merged

"hello world" windows port #5339

@RX14

This comment has been minimized.

Show comment
Hide comment
@RX14

RX14 Dec 1, 2017

Member

That patch has now become #5339.

I'll probably close this issue once it's merged. Although full or even basic windows support won't have been added by that PR, it'll be a slow slog of dissecting this current PR and putting it's bits in Crystal::System, which will take a while. So the most logical time to close this PR will be then, since it's obvious this isn't going to get merged as-is.

Member

RX14 commented Dec 1, 2017

That patch has now become #5339.

I'll probably close this issue once it's merged. Although full or even basic windows support won't have been added by that PR, it'll be a slow slog of dissecting this current PR and putting it's bits in Crystal::System, which will take a while. So the most logical time to close this PR will be then, since it's obvious this isn't going to get merged as-is.

@ylluminate

This comment has been minimized.

Show comment
Hide comment
@ylluminate

ylluminate Dec 1, 2017

Thanks @RX14. I know the Nanobox guys were thrilled to hear the progress and many are excited to be able to have this opportunity to start using Crystal broadly.

Thanks @RX14. I know the Nanobox guys were thrilled to hear the progress and many are excited to be able to have this opportunity to start using Crystal broadly.

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