"hello world" windows port #5339

RX14 · 2017-12-01T16:07:02Z

Add very minimal windows support using Crystal::System. The diff is much better viewed by looking at each commit seperately, and with?w=1 appended to the URL, which makes github ignore whitespace for diffs.

It builds on top of #5333 by adding lib_c bindings for windows, stubbing out the few remaining platform-specifics (reducing this is one of the main points of improvement), then implementing Errno, Time, Random, and FileDescriptor for windows. Currently the Time and Random implementations are stubbed out (the other reason why this is WIP, I need to copy the implementation of these from @txe).

I mainly PRed this to motivate merging #5333 by showing how it was used in my windows port.

You can try this by checking out this PR, then doing:

bin/crystal build --cross-compile --target x86_64-pc-windows-msvc -Dgc_none test.cr

with

# test.cr
puts "Hello World!"

This will print out a valid cl linker command which you can use after installing msvc. You will also have to compile pcre.lib using the instructions from #3582.

cc @txe, because this is the base on which you can finally start merging your port.

RX14 · 2017-12-01T19:31:09Z

I reduced the amount of stubbing-out i've done, and removed the windows_prelude in favour of just macroing the current prelude. Next is adding libgc support (I suspect this is easy)

ysbaddaden

Good job!

bew · 2017-12-01T21:36:12Z

src/crystal/system/win32/random.cr

+  end
+
+  def self.next_u : UInt8
+    # Chosen by fair dice roll, guaranteed to be random.


Let's not forget that 😄

Haha yeah, was nearly tempted to merge it after @ysbaddaden reviewed it positive.

Honestly that would be fine. Windows support is in flux. You can barely do anything with the corelib, neither IO, GC and I suspect the event loop.

This is a preliminary attempt. Fixes such as a proper random and time support could come in subsequent pull requests. I'll happily jump on implementing Random::Secure for example.

Sure, but i'd still rather merge something that compiled but raised instead of returning 4.

It compiles with gc support already (not managed to get it to run), and IO "works", there's just not really anywhere to get the file descriptors from (File not implemented).

Regardless, #5333 needs to get reviewed and merged first!

bew · 2017-12-01T21:49:42Z

src/file/stat.cr

@@ -93,8 +112,10 @@ class File
      io << ", rdev=0x"
      rdev.to_s(16, io)
      io << ", size=" << size
-      io << ", blksize=" << blksize
-      io << ", blocks=" << blocks
+      {% unless flag?(:win32) %}


Maybe add a comment saying that it's ignored on windows because those getter are currently not implemented and would raise?

bew · 2017-12-01T21:49:59Z

src/file/stat.cr

-        pp.comma
-        pp.text "blocks=#{blocks}"
-        pp.comma
+        {% unless flag?(:win32) %}


luislavena · 2017-12-01T23:47:08Z

Great work @RX14 on this! Definitely will take this weekend to review and send some comments!

Skimming through the code quickly say the constant being used for the platform is win32 and that made me a bit sad 😢.

win32 is a legacy reference to the pre-64bits days. Even if the API on Windows is referenced across the web as Win32 API, nowdays it represents more than just 32bits.

win32 might open for confusion about win64 and others.

Would be possible for you to use flag?(:windows) instead, like #3582?

Looking forward dig into this over the weekend.

Once again, thank you for your continuous contributions to Crystal!
❤️ ❤️ ❤️

RX14 · 2017-12-01T23:51:54Z

@luislavena no, windows is an operating system, win32 is an ABI. You can run the gnu ABI on windows just fine. See #4832 (comment) for @ysbaddaden explaining it.

straight-shoota · 2017-12-02T18:07:02Z

This is great! I was able to follow the steps and build a working windows program with Crystal =)

However, it does not only print hello world but also Not implemented (NotImplemented).
Is this expected/reproducable?
Seems like after the main method something else is called that has only been stubbed so far...

EDIT: I figured it is caused by Crystal::System::FileDescriptor#system_blocking= which raises NotImplemented

luislavena · 2017-12-02T18:32:34Z

Hello @RX14, thank you for your response, I don't disagree with @ysbaddaden comment, but it appears I missed when the changes to Crystal::Program#parse_flags was updated.

All good, once again thank you for your response.

straight-shoota · 2017-12-17T17:10:26Z

@RX14 Could you rebase this now that #5333 is merged?

RX14 · 2017-12-19T14:44:55Z

Rebased onto master after merging #5333.

Things left to do:

GC support
Make Time and Random stubs raise NotImplemented instead of return bogus.

If anyone wants to review (remember to use ?w=1 for a readable diff) this again before I implement those two go ahead.

straight-shoota · 2017-12-19T15:20:52Z

Do you intend to include GC here? Can't it be left as is using GC none and approach proper GC bindings in a separate PR?

RX14 · 2017-12-19T15:43:43Z

@straight-shoota the GC is easy. It's just a matter of compiling libgc for windows then linking it as-is. It probably works right now I just need to test it.

RX14 · 2017-12-19T20:54:04Z

OK, I've achieved those two goals. THis is ready to review + merge! I'm working on a .bat script which will automagically build libraries but until then, check out this fantastic comment by @txe on how to build pcre.lib and gc.lib manually.

RX14 · 2017-12-19T21:11:32Z

I just renamed NotImplemented to NotImplementedError and added a comment @bew wanted.

Sija · 2017-12-19T23:22:29Z

src/gc/none.cr

+
+    # :nodoc:
+    def self.pthread_join(thread : LibC::PthreadT) : Void*
+                                                     ret = LibC.pthread_join(thread, out value)


Indentation is off here.

So it is, fixed.

Shame it's impossible to get the formatter to work for macrocode.

ysbaddaden

Well, that looks good 👍

First real step to get win32 support. Now I'm eager for the follow up!

RX14 · 2017-12-20T13:02:55Z

🎉

RX14 · 2017-12-20T16:30:38Z

If anyone is going to start implementing more windows work (it's surprisingly easy), please leave a message in this thread before starting.

straight-shoota · 2017-12-20T19:09:45Z

Wouldn't it be better to have a separate issue for that? Maybe with a todo list to track progress?

Anyway, I'm happy to take a look at the time implementation 👍

RX14 · 2017-12-20T22:05:23Z

@straight-shoota you're right: let's use #26 to communicate about adding support instead.

straight-shoota · 2017-12-20T22:24:56Z

Hm, I explicitly suggested a new issue. #26 is already cluttered with irrelevant and outdated comments.

RX14 · 2017-12-20T23:17:15Z

Fiiine, go ahead and make a new one.

RX14 added pr:wip platform:windows labels Dec 1, 2017

RX14 changed the title ~~[WIP] Windows hello world~~ [WIP] "hello world" windows port Dec 1, 2017

RX14 added the kind:feature label Dec 1, 2017

RX14 force-pushed the feature/windows-hello-world branch from 13013ca to 4d20d19 Compare December 1, 2017 19:28

RX14 mentioned this pull request Dec 1, 2017

[WIP] Windows #3582

Closed

24 tasks

ysbaddaden approved these changes Dec 1, 2017

View reviewed changes

bew reviewed Dec 1, 2017

View reviewed changes

src/file/stat.cr

pp.comma

pp.text "blocks=#{blocks}"

pp.comma

{% unless flag?(:win32) %}

Copy link

Contributor

bew Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

RX14 force-pushed the feature/windows-hello-world branch from 4d20d19 to b4e847c Compare December 2, 2017 01:15

Cleanup unused lib_c requires

e8c09bb

RX14 force-pushed the feature/windows-hello-world branch from b4e847c to 29c9b9e Compare December 19, 2017 14:38

RX14 force-pushed the feature/windows-hello-world branch 2 times, most recently from 23ca0ee to 98b3e17 Compare December 19, 2017 20:10

RX14 changed the title ~~[WIP] "hello world" windows port~~ "hello world" windows port Dec 19, 2017

RX14 removed the pr:wip label Dec 19, 2017

RX14 added 2 commits December 19, 2017 21:10

Add NotImplementedError

db9493b

Add lib_c bindings for windows

70196cb

RX14 force-pushed the feature/windows-hello-world branch from 98b3e17 to 7139789 Compare December 19, 2017 21:10

RX14 force-pushed the feature/windows-hello-world branch from 7139789 to 4e4d9f3 Compare December 19, 2017 21:12

Sija reviewed Dec 19, 2017

View reviewed changes

ysbaddaden approved these changes Dec 20, 2017

View reviewed changes

RX14 added 5 commits December 20, 2017 12:47

Stub things out for windows

e404e1b

Implement Errno for win32

7579651

Add Crystal::System::Time stub for win32

3acecfd

Add Crystal::System::Random stub for win32

c6ae527

Implement Crystal::System::FileDescriptor for windows

eb60796

RX14 force-pushed the feature/windows-hello-world branch from 2b07a3d to eb60796 Compare December 20, 2017 12:47

RX14 merged commit 3b12082 into crystal-lang:master Dec 20, 2017

RX14 mentioned this pull request Dec 20, 2017

Support Windows as a first class platform (note: not WSL or emulation) #26

Open

straight-shoota mentioned this pull request Dec 21, 2017

Coordinate porting to Windows #5430

Open

22 tasks

RX14 added this to the Next milestone Dec 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"hello world" windows port #5339

"hello world" windows port #5339

RX14 commented Dec 1, 2017 •

edited

RX14 commented Dec 1, 2017 •

edited

ysbaddaden left a comment

bew Dec 1, 2017

RX14 Dec 1, 2017

ysbaddaden Dec 2, 2017

RX14 Dec 2, 2017 •

edited

bew Dec 1, 2017

bew Dec 1, 2017

luislavena commented Dec 1, 2017

RX14 commented Dec 1, 2017 •

edited

straight-shoota commented Dec 2, 2017 •

edited

luislavena commented Dec 2, 2017

straight-shoota commented Dec 17, 2017 •

edited

RX14 commented Dec 19, 2017 •

edited

straight-shoota commented Dec 19, 2017

RX14 commented Dec 19, 2017

RX14 commented Dec 19, 2017 •

edited

RX14 commented Dec 19, 2017

Sija Dec 19, 2017

RX14 Dec 19, 2017

ysbaddaden left a comment

RX14 commented Dec 20, 2017 •

edited

RX14 commented Dec 20, 2017

straight-shoota commented Dec 20, 2017

RX14 commented Dec 20, 2017 •

edited

straight-shoota commented Dec 20, 2017

RX14 commented Dec 20, 2017

"hello world" windows port #5339

"hello world" windows port #5339

Conversation

RX14 commented Dec 1, 2017 • edited

RX14 commented Dec 1, 2017 • edited

ysbaddaden left a comment

Choose a reason for hiding this comment

bew Dec 1, 2017

Choose a reason for hiding this comment

RX14 Dec 1, 2017

Choose a reason for hiding this comment

ysbaddaden Dec 2, 2017

Choose a reason for hiding this comment

RX14 Dec 2, 2017 • edited

Choose a reason for hiding this comment

bew Dec 1, 2017

Choose a reason for hiding this comment

bew Dec 1, 2017

Choose a reason for hiding this comment

luislavena commented Dec 1, 2017

RX14 commented Dec 1, 2017 • edited

straight-shoota commented Dec 2, 2017 • edited

luislavena commented Dec 2, 2017

straight-shoota commented Dec 17, 2017 • edited

RX14 commented Dec 19, 2017 • edited

straight-shoota commented Dec 19, 2017

RX14 commented Dec 19, 2017

RX14 commented Dec 19, 2017 • edited

RX14 commented Dec 19, 2017

Sija Dec 19, 2017

Choose a reason for hiding this comment

RX14 Dec 19, 2017

Choose a reason for hiding this comment

ysbaddaden left a comment

Choose a reason for hiding this comment

RX14 commented Dec 20, 2017 • edited

RX14 commented Dec 20, 2017

straight-shoota commented Dec 20, 2017

RX14 commented Dec 20, 2017 • edited

straight-shoota commented Dec 20, 2017

RX14 commented Dec 20, 2017

RX14 commented Dec 1, 2017 •

edited

RX14 commented Dec 1, 2017 •

edited

RX14 Dec 2, 2017 •

edited

RX14 commented Dec 1, 2017 •

edited

straight-shoota commented Dec 2, 2017 •

edited

straight-shoota commented Dec 17, 2017 •

edited

RX14 commented Dec 19, 2017 •

edited

RX14 commented Dec 19, 2017 •

edited

RX14 commented Dec 20, 2017 •

edited

RX14 commented Dec 20, 2017 •

edited