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

depexts for MSYS2 #5348

Merged
merged 3 commits into from Jan 9, 2023
Merged

depexts for MSYS2 #5348

merged 3 commits into from Jan 9, 2023

Conversation

jonahbeckford
Copy link
Contributor

MSYS2 on Windows uses the same package manager as Arch Linux (pacman). It targets several ABIs (compilers, bitiness and Windows C libraries).

This PR builds on the opam global variables that are registered when Diskuv OCaml (DKML) is installed. Those variables became available with DKML 1.1: https://gitlab.com/diskuv/diskuv-ocaml/-/blob/bd5b461b7d267ff897d053a67cce6ed5c26f27b4/CHANGES.md. Any MSYS2 distribution (not just DKML) can set those opam variables:

$ opam var --global
arch                 x86_64                                                         # Inferred from system
exe                  .exe                                                           # Suffix needed for executable filenames (Windows)
...
mingw-chost          x86_64-w64-mingw32                                             # Set through 'opam var'
mingw-package-prefix mingw-w64-clang-x86_64                                         # Set through 'opam var'
mingw-prefix         /clang64                                                       # Set through 'opam var'
msys2-nativedir      C:\Users\beckf\AppData\Local\Programs\DiskuvOCaml\tools\MSYS2\ # Set through 'opam var'
msystem              CLANG64                                                        # Set through 'opam var'
msystem-carch        x86_64                                                         # Set through 'opam var'
msystem-chost        x86_64-w64-mingw32                                             # Set through 'opam var'
msystem-prefix       /clang64                                                       # Set through 'opam var'
opam-version         2.2.0~alpha0~20221104                                          # The currently running opam version
...

Testing

With the following change to conf-pkg-config:

 depexts: [
   ["pkg-config"] {os-family = "debian"}
   ["pkgconf"] {os-distribution = "arch"}
   ["pkgconf-pkg-config"] {os-distribution = "fedora"}
   ["pkgconfig"] {os-distribution = "centos" & os-version <= "7"}
   ["pkgconf-pkg-config"] {os-distribution = "mageia"}
   ["pkgconfig"] {os-distribution = "rhel" & os-version <= "7"}
   ["pkgconfig"] {os-distribution = "ol" & os-version <= "7"}
   ["pkgconf"] {os-distribution = "alpine"}
   ["pkgconfig"] {os-distribution = "nixos"}
   ["devel/pkgconf"] {os = "openbsd"}
   ["pkg-config"] {os = "macos" & os-distribution = "homebrew"}
   ["pkgconfig"] {os = "macos" & os-distribution = "macports"}
   ["pkgconf"] {os = "freebsd"}
   ["pkgconf-pkg-config"] {os-distribution = "rhel" & os-version >= "8"}
   ["pkgconf-pkg-config"] {os-distribution = "centos" & os-version >= "8"}
   ["pkgconf-pkg-config"] {os-distribution = "ol" & os-version >= "8"}
   ["system:pkgconf"] {os = "win32" & os-distribution = "cygwinports"}
+  # Although not supported in Opam 2.2 (and earlier), we want the equivalent of:
+  #   ["%{mingw-package-prefix}%-pkg-config"] {os = "win32" & ?mingw-package-prefix}
+  ["mingw-w64-clang-i686-pkg-config"]   {os = "win32" & msystem = "CLANG32"}
+  ["mingw-w64-clang-x86_64-pkg-config"] {os = "win32" & msystem = "CLANG64"}
+  ["mingw-w64-i686-pkg-config"]         {os = "win32" & msystem = "MINGW32"}
+  ["mingw-w64-x86_64-pkg-config"]       {os = "win32" & msystem = "MINGW64"}
+  ["mingw-w64-ucrt-x86_64-pkg-config"]  {os = "win32" & msystem = "UCRT64"}
 ]

we can now do:

C:\> with-dkml `
  Z:\source\dkml-component-opam\_opam\share\dkml-component-staging-opam64\staging-files\windows_x86_64\bin\opam.exe `
  install Z:\source\opam-repository\packages\conf-pkg-config\conf-pkg-config.2\opam 
[conf-pkg-config.2] synchronised (no changes)
The following actions will be performed:
=== install 1 package
  ✶ conf-pkg-config 2 (pinned)

The following system packages will first need to be installed:
    mingw-w64-clang-x86_64-pkg-config

<><> Handling external dependencies <><><><><><><><><><><><><><><><><><><><><><>

opam believes some required external dependencies are missing. opam can:
> 1. Run C:\Users\beckf\AppData\Local\Programs\DiskuvOCaml\tools\MSYS2\usr\bin\pacman.exe to install them (may need root/sudo access)
  2. Display the recommended C:\Users\beckf\AppData\Local\Programs\DiskuvOCaml\tools\MSYS2\usr\bin\pacman.exe command and wait while you run it manually (e.g. in another
     terminal)
  3. Attempt installation anyway, and permanently register that this external dependency is present, but not detectable
  4. Abort the installation

[1/2/3/4] 1

+ C:\Users\beckf\AppData\Local\Programs\DiskuvOCaml\tools\MSYS2\usr\bin\pacman.exe "-Su" "--noconfirm" "mingw-w64-clang-x86_64-pkg-config"
- :: Starting core system upgrade...
-  there is nothing to do
- :: Starting full system upgrade...
- resolving dependencies...
- looking for conflicting packages...
- 
- Packages (1) mingw-w64-clang-x86_64-pkg-config-0.29.2-3
- 
- Total Installed Size:  1.39 MiB
- 
- :: Proceed with installation? [Y/n] 
- checking keyring...
- checking package integrity...
- loading package files...
- checking for file conflicts...
- :: Processing package changes...
- installing mingw-w64-clang-x86_64-pkg-config...

<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
▼ retrieved conf-pkg-config.2  (file://Z:/source/opam-repository/packages/conf-pkg-config/conf-pkg-config.2)
✶ installed conf-pkg-config.2
Done.
# Run eval $(opam env) to update the current shell environment

(If it wasn't a test the command line would simply be opam install conf-pkg-config)

Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

Thanks a lot! That looks great! My only issue is on the msys2-nativedir variable / msys2 detection story, but the rest looks ok.

src/state/opamSysInteract.ml Outdated Show resolved Hide resolved
src/state/opamSysInteract.ml Outdated Show resolved Hide resolved
src/state/opamSysInteract.ml Outdated Show resolved Hide resolved
@kit-ty-kate
Copy link
Member

kit-ty-kate commented Nov 11, 2022

Oh, also I think the os-distribution should be also set to msys2 (see how it's done for macOS (homebrew vs. macports) in src/state/opamSysPoll.ml). Otherwise we can't distinguish between msys2 and other packages managers.

@@ -130,6 +138,15 @@ let family ~env () =
"External dependency handling for macOS requires either \
MacPorts or Homebrew - neither could be found"
| "suse" | "opensuse" -> Suse
| "windows" ->
Copy link
Member

Choose a reason for hiding this comment

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

Something like that would also need to be done (see how it's done for BSDs above)

Suggested change
| "windows" ->
| "windows" ->
begin match OpamSysPoll.os_distribution env with
| Some "msys2" ->

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing! Did a search in fdopen for os-distribution = "win32"; nothing found, so it is safe for existing Diskuv users. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For posterity the setting of os-distribution was done in diskuv/dkml-runtime-distribution@ff42f08 (used by the Diskuv OCaml installer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the Some "msys2" on top of existing match. Once we decide on a global variable for the package manager binary (C:\Users\beckf\AppData\Local\Programs\DiskuvOCaml\tools\MSYS2\usr\bin\pacman.exe) then the match clause will simplify.

@jonahbeckford
Copy link
Contributor Author

Oh, also I think the os-distribution should be also set to msys2 (see how it's done for macOS (homebrew vs. macports) in src/state/opamSysPoll.ml). Otherwise we can't distinguish between msys2 and other packages managers.

Can you suggest to me what the values should be? I have no idea what the distinction is between os-distribution and os-family. Today it is:

opam-version         2.2.0~alpha0~20221104                                          # The currently running opam version
os                   win32                                                          # Inferred from system
os-distribution      win32                                                          # Inferred from system
os-family            windows                                                        # Inferred from system
os-version           10.0.22621                                                     # Inferred from system

Do you want:

opam-version         2.2.0~alpha0~20221104                                          # The currently running opam version
os                   win32                                                          # Inferred from system
os-distribution      msys2                                                          # Inferred from system
os-family            msys2                                                        # Inferred from system
os-version           10.0.22621                                                     # Inferred from system

Note: I've been using opam switch variables rather than opam global variables for the wrap-build-commands so that DKML created switches (ie. opam dkml init) can avoid conflicts with other Windows switches (I guess Cygwin). And I was going to use ?mingw-package-prefix to check for MSYS2 if needed. However perhaps it is time to ditch the interoperability ... either Opam will globally use MSYS2 or Opam will globally use Cygwin.


Also, which opam variable can be used to indicate which Windows C library (SDK+headers) to use when compiling C programs? The choices boil down to Windows SDK (I've been calling this native Windows) or the GNU C library (MinGW/Cygwin). Rust for example equates the binary choice to a choice in the ABI: https://rust-lang.github.io/rustup/installation/windows.html. It would be nice to have an opam variable that clearly said which Windows ABI to use. Today a package maintainer can look at the lack of the cygwinports opam value to compile for native Windows, but that is hacky. They can also look at the ocamlc -config ccomp_type for msvc, but that is insufficient because clang can compile both the Windows SDK and the GNU C library.

@kit-ty-kate
Copy link
Member

I think the values should be:

os-distribution: msys2
os-family: windows
os: win32

Also thinking about the msystem values in your first comment: do all packages have versions for each subsystems? If so maybe it would be better to be able to just do:

  ["pkg-config"] {os = "win32" & os-distribution = "msys2"}

and let opam infer which is the current subsystem (using either the MSYSTEM or MINGW_PACKAGE_PREFIX environment variable) or use the opam variable for that (whichever you prefer)

@jonahbeckford
Copy link
Contributor Author

I think the values should be:

os-distribution: msys2
os-family: windows
os: win32

Thanks.

Also thinking about the msystem values in your first comment: do all packages have versions for each subsystems? If so maybe it would be better to be able to just do:

  ["pkg-config"] {os = "win32" & os-distribution = "msys2"}

and let opam infer which is the current subsystem (using either the MSYSTEM or MINGW_PACKAGE_PREFIX environment variable) or use the opam variable for that (whichever you prefer)

Sigh; most but not all packages have versions for each subsystem. Common executables like bash and p7zip do not have subsystem packages since those tools are only for the build machine ABI. But almost anything that exposes a C library (including pkg-config) are only available as subsystem packages since C libraries by definition are ABI specific.

Perhaps we could use a label? Example:

["p7zip"]              {os = "win32" & os-distribution = "msys2"}
["rsync"]              {os = "win32" & os-distribution = "msys2"}
["abi:pkg-config"]     {os = "win32" & os-distribution = "msys2"}
["abi:SDL2"]           {os = "win32" & os-distribution = "msys2"}

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

Thanks for this!

We need to be concerned about OPAMCONFIRMLEVEL being respected - and we're going to be meeting shortly to discuss how to deal with the detection of MSYS2 in this case, so that can be added separately.

master_changes.md Outdated Show resolved Hide resolved
src/state/opamSysInteract.ml Outdated Show resolved Hide resolved
src/state/opamSysInteract.ml Outdated Show resolved Hide resolved
jonahbeckford added a commit to diskuv/dkml-runtime-distribution that referenced this pull request Nov 18, 2022
@rjbou
Copy link
Collaborator

rjbou commented Dec 15, 2022

After discussion, we introduce sys-pkg-manager-path as a variable that contains system package manager full path. It is needed in this PR for Msys2, and in the future will be needed for cygwin support.
I updated the PR consequently, if it ok for you @jonahbeckford @dra27 @kit-ty-kate , i'll rebase/clean history.

jonahbeckford added a commit to diskuv/dkml-runtime-distribution that referenced this pull request Dec 16, 2022
@jonahbeckford
Copy link
Contributor Author

After discussion, we introduce sys-pkg-manager-path as a variable that contains system package manager full path. It is needed in this PR for Msys2, and in the future will be needed for cygwin support. I updated the PR consequently, if it ok for you @jonahbeckford @dra27 @kit-ty-kate , i'll rebase/clean history.

Thanks. I added sys-pkg-manager-path (diskuv/dkml-runtime-distribution@45b9997 for posterity).

Looks good!

@dra27
Copy link
Member

dra27 commented Dec 19, 2022

Sorry, @jonahbeckford, we've just been discussing again trying to reconcile how this variable works for all the package managers, and in doing so we've come back round to something that's closer to where you started! For this PR, with apologies for yet another tweak needed in dkml-runtime, could the variable by sys-pkg-manager-cmd-msys2?

For a later PR, the plan is then to have sys-pkg-manager-cmd-cygwin (for Cygwin's installer) and to alter the rest of the function so that in theory you could define sys-pkg-manager-cmd-alpine, sys-pkg-manager-cmd-debian, etc. The previous sys-pkg-manager-path proposed having one variable shared by all package managers, but having a common prefix with an optional variable for each package manager seems better, especially in the case where you actually choose to change depext system (which can happen on macOS or with nix, and would mean that you could switch an entire opam root from MSYS2 to Cygwin or vice versa, if you really wanted).

In particular, that means that for everything which isn't Cygwin or MSYS2, opam would just use a default. i.e. on Debian/Ubuntu, you could choose to set sys-pkg-manager-cmd-debian to /usr/bin/apt-get, but if you don't then opam will just do apt-get as it does now but on Cygwin/MSYS2, you must the appropriate sys-pkg-manager-cmd-cygwin/sys-pkg-manager-cmd-msys2 or opam will instead say that it can't run the package manager.

jonahbeckford added a commit to diskuv/dkml-runtime-distribution that referenced this pull request Dec 20, 2022
@jonahbeckford
Copy link
Contributor Author

... could the variable by sys-pkg-manager-cmd-msys2?

No problem. diskuv/dkml-runtime-distribution@316f67e for posterity.

src/state/opamSysInteract.ml Outdated Show resolved Hide resolved
@rjbou rjbou moved this from PR in progress to PR finalised (merge with CI) in Opam 2.2.0 Jan 6, 2023
@rjbou rjbou added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Jan 6, 2023
@rjbou
Copy link
Collaborator

rjbou commented Jan 6, 2023

gentoo fix in #5410

@dra27 dra27 merged commit 9ff2373 into ocaml:master Jan 9, 2023
Opam 2.2.0 automation moved this from PR finalised (merge with CI) to Done Jan 9, 2023
@dra27
Copy link
Member

dra27 commented Jan 9, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AREA: DEPEXTS AREA: PORTABILITY PR: QUEUED Pending pull request, waiting for other work to be merged or closed
Projects
Opam 2.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants