Skip to content
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

increase the support version requirement for MacOS #17903

Closed
wants to merge 1 commit into from

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Apr 30, 2021

macOS 10.12 was born at June 13, 2016, and the end of support date is October 2019(ref https://en.wikipedia.org/wiki/MacOS_Sierra)

This version introduces many useful features.

For example, getentropy, clock_gettime which is beneficial. See also zeromq/libzmq#2175

Related: #17901

However we don't have a reliable way to use different system apis for different system versions. More terribly, it makes it hard to register these apis in the VM because it causes building latest Nim to fail.

see #17370

So maybe we should increase the support version requirement for MacOS

macOS 10.12 was born at June 13, 2016, and the end of support date is October 2019.

This version introduces more useful features.

For example, `getentropy`, `clock_gettime` which is benefit.

However we don't have a reliable way to use different system apis for different system versions. More terribly, it makes it hard to register these apis in the VM because it causes building latest Nim to fail.

see #17370

So maybe we should increase the support version requirement for MacOS
@timotheecour
Copy link
Member

this reminds me of #17378, which removed a useful functionality (rand in VM should be brought back IMO and #17378 should be done differently); note that this isn't specific to osx, any platform/OS will have some kind of versioning that should be handled for presence of an API (including nodejs etc). instead of upping the requirement, how about auto-detecting which os version we're compiling on and using a workaround when that version doesn't support the new API?

@ringabout
Copy link
Member Author

ringabout commented Apr 30, 2021

how about auto-detecting which os version we're compiling on and using a workaround when that version doesn't support the new API?

Yeah, I agree, but I have no idea how to do it. Or just use C header to handle the tricky things or weak linkage.

@timotheecour
Copy link
Member

Yeah, I agree, but I have no idea how to do it. Or just use C header to handle the tricky things or weak linkage.

  • this works:
proc main()=
  const (ver, status) = gorgeEx("/usr/bin/sw_vers -productVersion")
  doAssert status == 0
  echo ver # 11.2.3 # (on my osx)
main()

(more fine grained API availability based on looking at headers, macro definitions etc could be handled too, eg via proposal 5 from nim-lang/RFCs#205 const CVAR {.importc.}: int, but in this case it's not needed)

  • in other similar instances, (nodejs, linux, etc), we'll always have some similar way to get to this type of information at CT

this could be added for now to some std/private/platformutils

@ringabout
Copy link
Member Author

ringabout commented Apr 30, 2021

proc main()=
  const (ver, status) = gorgeEx("/usr/bin/sw_vers -productVersion")
  doAssert status == 0
  echo ver # 11.2.3 # (on my osx)
main()

Is it fast enough(consisting of more system calls)?

Maybe register it on VM using system calls?

@ringabout
Copy link
Member Author

So I think we should register osinfo api regarding Macos on vm first to solve the issues.

https://github.com/nim-lang/osinfo/blob/master/src/osinfo/posix.nim

@ringabout ringabout closed this Apr 30, 2021
@ringabout ringabout deleted the xflywind-patch-2 branch April 30, 2021 08:26
@ringabout
Copy link
Member Author

Indeed, it is already used at std/terminal thought at RT instead of CT. Let's generalize it!

  when defined(windows):
    var
      ver: OSVERSIONINFO
    ver.dwOSVersionInfoSize = sizeof(ver).DWORD
    let res = getVersionExW(addr ver)
    if res == 0:
      term.trueColorIsSupported = false
    else:
      term.trueColorIsSupported = ver.dwMajorVersion > 10 or
        (ver.dwMajorVersion == 10 and (ver.dwMinorVersion > 0 or
        (ver.dwMinorVersion == 0 and ver.dwBuildNumber >= 10586)))
    if not term.trueColorIsSupported:
      term.trueColorIsSupported = getEnv("ANSICON_DEF").len > 0

@timotheecour
Copy link
Member

timotheecour commented Apr 30, 2021

So I think we should register osinfo api regarding Macos on vm first to solve the issues.

sure, that works; but note that the API in https://github.com/nim-lang/osinfo/blob/master/src/osinfo/posix.nim itself is questionable; my advice is to for now introduce the API's we need as a private-for-now module (std/private/osinfo or std/private/platformutils), add vmops for what's needed at CT (but see note below regarding gorgeEx), and start using it in stdlib/compiler as needed; then later we can worry about exposing those in a proper module.

(eg, /usr/bin/sw_vers -productVersion is likely more efficient than what's currently done in osinfo/posix)

Is it fast enough(consisting of more system calls)?

well, regardless of whether there's a vmops or direct call to gorgeEx, there's a system call no matter what, but the gorgeEx is more flexible / easier to hack and doesn't need re-compiling nim, which is a big advantage. I'd prefer sticking to gorgeEx unless you can find a good counter-argument.

Performance wise, i checked, it's negligible (at least with /usr/bin/sw_vers -productVersion), which isn't surprising.

EDIT

i guess uname would make sense to have a vmops for, since it's useful posix-wide at least

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants