Task list for V3 - Githubissues

jonahbeckford commented 2 years ago

These tasks come from https://discuss.ocaml.org/t/ann-dkml-c-probe-2-0-0-cross-compiler-friendly-definitions-for-c-compiling/9950

[x] Update V2 README.md ... it is wrongly saying the API is (t_os, Rresult.msg) result rather than (t_os, string) result
[x] Remove unnecessary rresult (which wasn't even used in V2)
[x] Create V3 module and version
[x] Remove failwith
[x] Add Unknown to t_abi and other sum types
[x] Avoid compiler issues by defaulting to Unknown in the C header
[x] Decide if *BSD treated as Unknown or fully-supported
[x] Remove astring dependency
[x] Add C_conf module for C library and header locations during cross-compiling
[x] Add BSD testing in GitHub Actions
[x] Remove lazy
[x] Add cross-compiling for macOS on CI

jonahbeckford commented 2 years ago

The tests pass for these changes. Will keep this issue open for a bit in case anyone wants to modify the API before "V3" is finalized.

mseri commented 2 years ago

Could we also add BSDs? According to https://web.archive.org/web/20180331065236/http://nadeausoftware.com/articles/2012/01/c_c_tip_how_use_compiler_predefined_macros_detect_operating_system#BSD the available macros are __DragonFly__, __FreeBSD__, __NetBSD__ and __OpenBSD__. We have a growing group of OpenBSD and FreeBSD users in the community (don't know about the other two)

jonahbeckford commented 2 years ago

I assume these are *BSD x86_64 users? (need to know valid ABI=OS+ARCH combinations)

Are you suggesting mapping *BSD to Unknown? In reality the mapping is unnecessary since the default mapping is Unknown. I explicitly mapped PPC64 and S390X to Unknown only to aid a future PPC64/S390X maintainer who could maintain the testing apparatus (ex. Docker+QEMU).

If you are suggesting mapping BSD to real values, any chance you can message one of the OpenBSD developers and one of the FreeBSD developers and ask them to be a maintainer, or at least to be a release tester? Can't use non-Linux kernels in Docker so it is much harder to test BSD, and this code base will quickly become unsupportable.

jonahbeckford commented 2 years ago

(Stumbled across https://github.com/cross-platform-actions/action which could be used to test OpenBSD and FreeBSD. Would prefer the help of someone who actually uses BSD of course.)

mseri commented 2 years ago

I assume these are *BSD x86_64 users? (need to know valid ABI=OS+ARCH combinations)

Yes, as far as I am aware.

(Stumbled across https://github.com/cross-platform-actions/action which could be used to test OpenBSD and FreeBSD. Would prefer the help of someone who actually uses BSD of course.)

Oh nice. I think it would be nice to add x86_64 BSDs and use unknown for all the rest. Anything else could be added once people have specific needs. But I don't personally have any strong feeling in adding them now or just waiting until somebody needs them, and maybe it makes sense to avoid unnecessary work unless there is a strong push for it. After all, for now, the unknown option seems to satisfy the needs

jonahbeckford commented 2 years ago

I'm testing on OpenBSD manually right now.

Currently, with some committed bugfixes but without any BSD detection logic, OpenBSD gives:

(** New applications should use the {!V3} module instead. *)
module V1 = struct
  type t_os = Android | IOS | Linux | OSX | Windows
  type t_abi =
              | Android_arm64v8a
              | Android_arm32v7a
              | Android_x86
              | Android_x86_64
              | Darwin_arm64
              | Darwin_x86_64
              | Linux_arm64
              | Linux_arm32v6
              | Linux_arm32v7
              | Linux_x86_64
              | Windows_x86_64
              | Windows_x86
              | Windows_arm64
              | Windows_arm32

  let get_os : (t_os, string) result Lazy.t = lazy (Result.error ("'UnknownOS' OS is only available in Target_context.V3 or later"))
  let get_abi : (t_abi, string) result Lazy.t = lazy (Result.error ("'Unknown_unknown' ABI is only available in Target_context.V3 or later"))
  let get_abi_name : (string, string) result Lazy.t = lazy (Result.error ("'Unknown_unknown' ABI is only available in Target_context.V3 or later"))
end (* module V1 *)

(** New applications should use the {!V3} module instead. *)
module V2 = struct
  type t_os = Android | IOS | Linux | OSX | Windows
  type t_abi =
              | Android_arm64v8a
              | Android_arm32v7a
              | Android_x86
              | Android_x86_64
              | Darwin_arm64
              | Darwin_x86_64
              | Linux_arm64
              | Linux_arm32v6
              | Linux_arm32v7
              | Linux_x86_64
              | Linux_x86
              | Windows_x86_64
              | Windows_x86
              | Windows_arm64
              | Windows_arm32

  let get_os : (t_os, string) result Lazy.t = lazy (Result.error ("'UnknownOS' OS is only available in Target_context.V3 or later"))
  let get_abi : (t_abi, string) result Lazy.t = lazy (Result.error ("'Unknown_unknown' ABI is only available in Target_context.V3 or later"))
  let get_abi_name : (string, string) result Lazy.t = lazy (Result.error ("'Unknown_unknown' ABI is only available in Target_context.V3 or later"))
end (* module V2 *)

(** Enumerations of the operating system and the ABI, typically from an introspection of OCaml's native C compiler. *)
module V3 = struct
  type t_os = UnknownOS | Android | IOS | Linux | OSX | Windows
  type t_abi =
              | Unknown_unknown
              | Android_arm64v8a
              | Android_arm32v7a
              | Android_x86
              | Android_x86_64
              | Darwin_arm64
              | Darwin_x86_64
              | Linux_arm64
              | Linux_arm32v6
              | Linux_arm32v7
              | Linux_x86_64
              | Linux_x86
              | Windows_x86_64
              | Windows_x86
              | Windows_arm64
              | Windows_arm32

  let get_os : (t_os, string) result Lazy.t = lazy (Result.ok (UnknownOS))
  let get_abi : (t_abi, string) result Lazy.t = lazy (Result.ok (Unknown_unknown))
  let get_abi_name : (string, string) result Lazy.t = lazy (Result.ok ("unknown_unknown"))
end (* module V3 *)

jonahbeckford commented 2 years ago

After the last commit OpenBSD now gives:

(** Enumerations of the operating system and the ABI, typically from an introspection of OCaml's native C compiler. *)
module V3 = struct
  type t_os =
              | UnknownOS
              | Android
              | DragonFly
              | FreeBSD
              | IOS
              | Linux
              | NetBSD
              | OpenBSD
              | OSX
              | Windows

  type t_abi =
              | Unknown_unknown
              | Android_arm32v7a
              | Android_arm64v8a
              | Android_x86
              | Android_x86_64
              | Darwin_arm64
              | Darwin_x86_64
              | DragonFly_x86_64
              | FreeBSD_x86_64
              | Linux_arm32v6
              | Linux_arm32v7
              | Linux_arm64
              | Linux_x86
              | Linux_x86_64
              | NetBSD_x86_64
              | OpenBSD_x86_64
              | Windows_arm32
              | Windows_arm64
              | Windows_x86
              | Windows_x86_64

  let get_os : (t_os, string) result Lazy.t = lazy (Result.ok (OpenBSD))
  let get_abi : (t_abi, string) result Lazy.t = lazy (Result.ok (OpenBSD_x86_64))
  let get_abi_name : (string, string) result Lazy.t = lazy (Result.ok ("openbsd_x86_64"))
end (* module V3 *)

So I'm confident in the BSD detection logic.

Also, now that I'm looking at the API, I think I should drop lazy in V3 and just use a normal function. So it would be:

  let get_os : unit -> (t_os, string) result = fun () -> (Result.ok (OpenBSD))
  let get_abi : unit -> (t_abi, string) result = fun () -> (Result.ok (OpenBSD_x86_64))
  let get_abi_name : unit -> (string, string) result = fun () -> (Result.ok ("openbsd_x86_64"))

mseri commented 2 years ago

Fantastic! Thank you very much. Yeah, I agree that the lazy there seems unnecessary

jonahbeckford commented 2 years ago

I've got all the changes I want done. @mseri you mentioned in https://discuss.ocaml.org/t/ann-dkml-c-probe-2-0-0-cross-compiler-friendly-definitions-for-c-compiling/9950/5?u=jbeckford that you were thinking of doing a PR. I've already removed the extra OCaml packages, but what I have not done is lower the supported OCaml version. I think 4.02 is needed for eigen, so that would require a PR.

mseri commented 2 years ago

I had done it (https://github.com/diskuv/dkml-c-probe/pull/2) and also closed it, since in the meantime you had a nicer approach with seq already committed :)

mseri commented 2 years ago

Eigen is used only by owl which used 4.10 and in master now require 4.14, v3 should be good to be used there

jonahbeckford commented 2 years ago

Ack; don't know why I missed your PR.

I'll submit this issue to Opam. Thx

diskuv / dkml-c-probe

Task list for V3 #1