Skip to content

Conversation

@MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Jan 4, 2021

This pull request:

  • updates vendored dependencies to their latest commit (on master branch) (required for client bug-fixes);
  • updates the minimum OCaml version to 4.12;
  • updates the requirement on Lwt to 5.4.1 for Windows bugfixes;
  • adds a pipeline for building Windows Docker images containing fdopen's Docker for Windows Opam and OCaml installations;
  • allows to run the client on Windows.

Building Windows Docker images requires OCluster workers running on Windows (see OCluster PR 128) are used. The current pipeline doesn't create multiarch images for different OS (i.e., an image containing Linux and Windows images). It doesn't change the current Linux tags in any way.

It uses three new distros: windows-mingw, windows-msvc, and cygwin, corresponding to the three OCaml Windows ports. Cygwin is currently disabled until newer versions of OCaml (4.12.1 ?) and Opam (2.1 ?) are made available.

The pushed Windows tags have the current scheme, where <distro> is one of windows-mingw or windows-msvc:

  • <distro>-<win_version>-opam;
  • <distro>-<win_version>-ocaml-<ocaml_version>;
  • <distro>-<win_version> -> <distro>-<win_version>-ocaml-<ocaml_version_latest>;
  • <distro>-ocaml-<ocaml_version>, a multiarch image containing all <distro>-<win_version>-ocaml-<ocaml_version> for each win_version.
  • <distro> , a multiarch image containing all <distro>-<win_version> for each <win_version>.

The <win_version> corresponds to the mcr.microsoft.com/windows:<win_version> base image.

Some possible set of tags that aren't in this PR is:

  • <distro>-opam, a multiarch image containing all <distro>-<win_version>-opam for all win_version, but as opam images are intermediary, we don't have to bother with it.

A image is generated for each version of Windows 10 (LTSC and Semi Annual releases only). Images for earlier versions of Windows 10 are not available. As Microsoft doesn't provide a "latest" tags for its images, we won't either provide an image based on the "latest" version of Windows.

Depends on :

Might depend on (but not sure as I haven't changed the API used):

@MisterDA MisterDA force-pushed the windows branch 2 times, most recently from 43b043b to d34095b Compare January 4, 2021 14:50
@MisterDA MisterDA force-pushed the windows branch 8 times, most recently from e60fbf0 to d18cc8d Compare January 19, 2021 16:21
@MisterDA MisterDA force-pushed the windows branch 4 times, most recently from 6c24817 to 09c6cdb Compare February 1, 2021 10:11
@MisterDA MisterDA force-pushed the windows branch 3 times, most recently from aa7b7b3 to d45fb3b Compare February 2, 2021 17:09
@MisterDA MisterDA force-pushed the windows branch 2 times, most recently from fbdc2aa to 8f43838 Compare March 4, 2021 17:46
@avsm
Copy link
Member

avsm commented Mar 6, 2021

All the prerequisites for this are now merged.

@MisterDA MisterDA force-pushed the windows branch 5 times, most recently from 26a3e05 to 752ceee Compare March 24, 2021 10:32
@MisterDA MisterDA marked this pull request as ready for review March 24, 2021 10:44
@talex5
Copy link
Contributor

talex5 commented Mar 24, 2021

CI is failing. Probably the Dockerfile needs updating.

@talex5
Copy link
Contributor

talex5 commented Jun 24, 2021

I split out some commits to #119 and #120. #119 is merged and I rebased on that. If you're happy with #120 we can merge and rebase on that too. That should leave just Windows stuff in this PR.

@avsm
Copy link
Member

avsm commented Jun 29, 2021

Winget Pr merged into dockerfile

Copy link
Contributor

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Thanks, this is looking very good!

I rebased it on master to get the new diff test. There are two changes to the Linux builds that are probably accidental. I'm not qualified to say whether the Windows builds work.

Things to do before merging:

  • Undo the changes to the Linux builds.
  • @MisterDA mentioned that ocaml-dockerfile should be updated to remove the [TMP] bit in the last commit).

src/pipeline.ml Outdated
cmd "bash" @@
maybe_install_secondary_compiler run os_family switch @@
entrypoint_exec (Option.to_list personality @ opam_exec) @@
match os_family with `Linux | `Cygwin -> cmd "bash" | `Windows -> cmd_exec ["cmd.exe"] @@
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
match os_family with `Linux | `Cygwin -> cmd "bash" | `Windows -> cmd_exec ["cmd.exe"] @@
(match os_family with `Linux | `Cygwin -> cmd "bash" | `Windows -> cmd_exec ["cmd.exe"]) @@

Otherwise it's doing something different ;-)

RUN opam install -y opam-depext
RUN opam pin add -k version ocaml-base-compiler 4.02.3
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this change to the order of the commands intentional?

@talex5 talex5 mentioned this pull request Jul 8, 2021
MisterDA added 6 commits July 8, 2021 15:11
If the docker build uses `--squash`, it'll stop with:

```
error squashing image: error registering layer: re-exec error: exit status 1: output: hcsshim::ImportLayer - failed failed in Win32: The system cannot find the path specified. (0x3)
```
Error was:

    base_images: internal error, uncaught exception:
                 Unix.Unix_error(Unix.EINVAL, "select", "")
@talex5
Copy link
Contributor

talex5 commented Jul 9, 2021

Deployed live, but some of the builds failed.

https://base-images.ocamllabs.io/job/2021-07-08/165647-ocluster-build-fde59f:

#=== ERROR while compiling ocaml-variants.4.09.1+mingw64 ======================#
# /bin/dash: 1: iconv: not found
# make[4]: *** [../../Makefile.common:71: getserv.o] Error 1

https://base-images.ocamllabs.io/job/2021-07-08/165647-ocluster-build-65fbee:

[ERROR] The sources of the following couldn't be obtained, aborting:
          - config-file.1.2: curl error code       0 [main] curl 834 child_info_fork::abort: \??\C:\cygwin64\bin\cygssl-1.1.dll: Loaded to different address: parent(0x3FE3A0000) != child(0x400000)
          - depext-cygwinports.0.0.8: curl error code       0 [main] curl 835 child_info_fork::abort: \??\C:\cygwin64\bin\cygssl-1.1.dll: Loaded to different address: parent(0x3FE3A0000) != child(0x400000)

https://base-images.ocamllabs.io/job/2021-07-08/165647-ocluster-build-dfdae0:

# File "C:\opam\.opam\4.06\.opam-switch\build\ocaml-variants.4.06.1+mingw64\middle_end\inlining_decision.ml", line 1:
# Error: Assembler error, input left in file C:\cygwin64\tmp\camlasmc2b619.s
# make[1]: *** [Makefile:1297: middle_end/inlining_decision.cmx] Error 2

@talex5
Copy link
Contributor

talex5 commented Jul 9, 2021

After clicking the rebuild buttons a few times, everything is now passing.

@talex5 talex5 merged commit e4043eb into ocurrent:master Jul 9, 2021
@MisterDA MisterDA deleted the windows branch April 22, 2022 10:56
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.

4 participants