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

Implement System for Win32 #6972

Merged
merged 14 commits into from Dec 17, 2018

Conversation

@markrjr
Copy link
Contributor

markrjr commented Oct 21, 2018

Pretty simple implementation of the hostname and cpu count functions.

@r00ster91

This comment has been minimized.

Copy link

r00ster91 commented on src/crystal/system/win32/hostname.cr in aa43722 Oct 21, 2018

Error messages normally don't end with a dot.

@r00ster91

This comment has been minimized.

Copy link

r00ster91 commented on src/lib_c/x86_64-windows-msvc/c/ntsysteminfo.cr in aa43722 Oct 21, 2018

I think these comments should be removed (ditto below) unless we link all other functions too. And it really doesn't take too long to search them yourself. I for example just need to hover "GetSystemInfo" and can directly search for it and the link is my first result.

@Sija

This comment has been minimized.

Copy link
Contributor

Sija commented Oct 21, 2018

crystal tool format please

@straight-shoota
Copy link
Member

straight-shoota left a comment

Please take a look at https://github.com/crystal-lang/crystal/wiki/Porting-to-Windows#bindings for naming in C bindings.

Also, you should put them in a file with the same file name as the C header filer (just with .cr extension). Ditto for the other features in this file.

Show resolved Hide resolved src/lib_c/x86_64-windows-msvc/c/ntsysteminfo.cr Outdated
Show resolved Hide resolved src/lib_c/x86_64-windows-msvc/c/ntsysteminfo.cr Outdated
Show resolved Hide resolved src/lib_c/x86_64-windows-msvc/c/ntsysteminfo.cr Outdated
Show resolved Hide resolved src/lib_c/x86_64-windows-msvc/c/ntsysteminfo.cr Outdated
@markrjr

This comment has been minimized.

Copy link
Contributor Author

markrjr commented Oct 24, 2018

I think I hit all the PR comments with fixes in the latest push. Let me know if there's anything else.

Show resolved Hide resolved src/crystal/system/win32/hostname.cr Outdated
Show resolved Hide resolved src/crystal/system/win32/hostname.cr Outdated
Show resolved Hide resolved src/lib_c/x86_64-windows-msvc/c/sysinfoapi.cr Outdated
Show resolved Hide resolved src/lib_c/x86_64-windows-msvc/c/int_safe.cr Outdated
Show resolved Hide resolved src/lib_c/x86_64-windows-msvc/c/winnt.cr Outdated

@straight-shoota straight-shoota referenced this pull request Dec 4, 2018

Open

Coordinate porting to Windows #5430

12 of 21 tasks complete
@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Dec 4, 2018

@markrjr This PR needs to address the review comments.

@markrjr

This comment has been minimized.

Copy link
Contributor Author

markrjr commented Dec 4, 2018

I've got most of it done locally, but have not had time to finish the last bits. I'll commit by the end of the week.

markrjr added some commits Dec 7, 2018

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Dec 9, 2018

Looks great, @markrjr !

It still needs to be integrated with src/crystal/system.cr. Something like this:

{% if flag?(:unix) %}
  require "./system/unix/hostname"

  {% if flag?(:freebsd) || flag?(:openbsd) %}
  require "./system/unix/sysctl_cpucount"
  {% else %}
    require "./system/unix/sysconf_cpucount"
  {% end %}
{% elsif flag?(:win32) %}
  require "./system/win32/hostname"
  require "./system/win32/cpucount"
{% else %}
  {% raise "No Crystal::System implementation available" %}
{% end %}

Specs would be nice. Unfortunately, Process is not yet ported to win32 so it's currently not possible to shell out to retrieve verification values in system_spec.cr.

As a workaround, the commands can be executed as macros. This means the spec will only pass when executed on the same machine as it was compiled. This works on WSL, but not when cross-compiling from a different machine. Such a hack should be put into the PR, but it serves for manual verification.

Once Process is ported, shelling out to hostname should just work on windows as well.

On windows, the cpu count is actually available as environment variable NUMBER_OF_PROCESSORS. The cpu count spec could already be adjusted to use that env var for verification.

Show resolved Hide resolved spec/std/system_spec.cr Outdated
Show resolved Hide resolved spec/std/system_spec.cr Outdated
Show resolved Hide resolved src/crystal/system.cr Outdated

markrjr added some commits Dec 11, 2018

Show resolved Hide resolved src/crystal/system.cr Outdated
Add indentation.
Co-Authored-By: markrjr <markrjr@users.noreply.github.com>
@markrjr

This comment has been minimized.

Copy link
Contributor Author

markrjr commented Dec 12, 2018

Is there anything else to tackle here? @RX14 @straight-shoota @r00ster91

Show resolved Hide resolved src/crystal/system.cr Outdated
@RX14

RX14 approved these changes Dec 15, 2018

Show resolved Hide resolved src/crystal/system/win32/hostname.cr Outdated
@RX14

RX14 approved these changes Dec 16, 2018

@RX14

RX14 approved these changes Dec 16, 2018

@markrjr

This comment has been minimized.

Copy link
Contributor Author

markrjr commented Dec 16, 2018

Neat! 🎉I will do a bit more of a dive into the language before I submit another PR, but thanks for all the help everyone!

@RX14 RX14 requested a review from bcardiff Dec 16, 2018

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Dec 16, 2018

@markrjr You're initial patch wasn't bad. Keep it on! =)

@RX14 RX14 added this to the 0.27.1 milestone Dec 17, 2018

@RX14 RX14 merged commit d8ab28e into crystal-lang:master Dec 17, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@markrjr markrjr deleted the markrjr:markrjr/windows_system branch Dec 17, 2018

@sdogruyol

This comment has been minimized.

Copy link
Member

sdogruyol commented Dec 17, 2018

Thanks for the great first patch @markrjr 🎉

@straight-shoota straight-shoota added this to Todo in Windows via automation Jan 8, 2019

@straight-shoota straight-shoota moved this from Todo to Stdlib in Windows Jan 8, 2019

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