Skip to content

Conversation

@neatorobito
Copy link
Contributor

Pretty simple implementation of the hostname and cpu count functions.

@Sija
Copy link
Contributor

Sija commented Oct 21, 2018

crystal tool format please

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

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.

@neatorobito
Copy link
Contributor Author

neatorobito 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.

@straight-shoota straight-shoota mentioned this pull request Dec 4, 2018
22 tasks
@straight-shoota
Copy link
Member

@markrjr This PR needs to address the review comments.

@neatorobito
Copy link
Contributor Author

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.

@straight-shoota
Copy link
Member

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.

Co-Authored-By: markrjr <[email protected]>
@neatorobito
Copy link
Contributor Author

neatorobito commented Dec 12, 2018

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

@RX14 RX14 added kind:feature topic:stdlib platform:windows Windows support based on the MSVC toolchain / Win32 API labels Dec 16, 2018
@neatorobito
Copy link
Contributor Author

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 December 16, 2018 21:38
@straight-shoota
Copy link
Member

@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
@neatorobito neatorobito deleted the markrjr/windows_system branch December 17, 2018 08:13
@sdogruyol
Copy link
Member

Thanks for the great first patch @markrjr 🎉

straight-shoota added a commit to straight-shoota/crystal that referenced this pull request Jan 7, 2020
System methods have been implemented for win32 in crystal-lang#6972
straight-shoota added a commit that referenced this pull request Jan 7, 2020
System methods have been implemented for win32 in #6972
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:feature platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:system

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants