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.

Sign up for GitHub

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

Jump to bottom

Add CI for MSVC using dkml-workflows #6540

Merged
merged 7 commits into from
Nov 29, 2022
Merged

Add CI for MSVC using dkml-workflows #6540

merged 7 commits into from
Nov 29, 2022

Conversation

jonahbeckford
Copy link
Collaborator

Implements #6535

jonahbeckford
jonahbeckford commented Nov 22, 2022
View reviewed changes
CHANGES.md Outdated Show resolved Hide resolved
anmonteiro
anmonteiro reviewed Nov 22, 2022
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@rgrinberg
Copy link
Member

Would it be possible to remove the stuff that doesn't pertain to msvc?

@jonahbeckford
Copy link
Collaborator Author

Would it be possible to remove the stuff that doesn't pertain to msvc?

Is "the stuff" talking about the gh-darwin/gh-linux folders? Sure, we can remove files that were generated during dune promote with a Makefile rule. Is that fine?

@rgrinberg
Copy link
Member

Yes, that is what I meant. Removing them in a makefile rule is fine

@jonahbeckford
Copy link
Collaborator Author

Yes, that is what I meant. Removing them in a makefile rule is fine

Done.

And I'd like to see how long the MSVC builds take now that they've had a chance to cache some parts.

@rgrinberg
Copy link
Member

@nojb can you review please?

nojb
nojb approved these changes Nov 23, 2022
View reviewed changes
Copy link
Collaborator

@nojb nojb left a comment

Choose a reason for hiding this comment

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

I have to confess that I find the amount of generated code a bit scary, but the results are good: the CI jobs (one for 32-bit and one for 64-bit) each take ~10m to complete once the cache is full.

The generated code itself is hard to review, but here are some general questions:

  • What is the version of MSVC targeted?
  • What is the workflow to update the generated files? make update-dkml?
  • What is the required environment to execute the desktop-ci-* targets in the main Makefile?

For the record, could we enumerate somewhere which files are generated and which files are supposed to be maintained by hand? In particular, build-test.sh is hand-written, right?

I am approving based on the general idea that this is an strict improvement on the present situation (no MSVC testing at all).

@jonahbeckford
Copy link
Collaborator Author

jonahbeckford commented Nov 23, 2022 •
edited
Loading

NOTE: Everything below is now in a FAQ in a README in the PR.

  • What is the version of MSVC targeted?

There is no "target" per se. The CI machine (or your developer machine) is queried for all MSVC versions, and any versions that I know to be compatible with OCaml are used. The precise definition is Get-CompatibleVisualStudios at https://github.com/diskuv/dkml-runtime-distribution/blob/main/src/windows/Machine/Machine.psm1; that package is available for common use in the central Opam repository.

The compatible versions today are: Visual Studio 2019 with a) Windows 10 SDK (10.0.18362.0) and b) MSVC v142 - VS 2019 C++ x64/x86 build tools (v14.25) or (v14.26). Machine.psm1 has notes why other MSVC 2019 versions are not compatible with OCaml; very much beyond the scope of this PR though!

More importantly, just running it will show the user what is on their system and what Visual Studio components they are missing (if any).

  • What is the workflow to update the generated files? make update-dkml?

Yes. Obviously assuming there is an update available from opam update.

  • What is the required environment to execute the desktop-ci-* targets in the main Makefile?

Same as first answer.

For the record, could we enumerate somewhere which files are generated and which files are supposed to be maintained by hand? In particular, build-test.sh is hand-written, right?

To make it easy I'll put a README.md in ci/setup-dkml/: only those files are auto-generated.

I'll add in that README a link to https://github.com/diskuv/dkml-workflows#readme (setup-dkml) which a) describes what is hand-generated and what goes into it, and b) includes auto-tested examples and c) implicitly defines a place for bug reports.

jonahbeckford and others added 5 commits November 23, 2022 12:05
@jonahbeckford
Add CI for MSVC using dkml-workflows
592e5a1 
Implements ocaml#6535

Signed-off-by: Jonah Beckford <[email protected]>
@jonahbeckford
Update CHANGES.md
8a3e8bb 
Signed-off-by: jonahbeckford <[email protected]>
@jonahbeckford @anmonteiro
Update Makefile
80c8ddd 
Co-authored-by: Antonio Nuno Monteiro <[email protected]>
Signed-off-by: jonahbeckford <[email protected]>
@jonahbeckford
Remove darwin and linux files from DKML CI
8960c27 
Signed-off-by: Jonah Beckford <[email protected]>
@jonahbeckford
Add README.md for setup-dkml
363d8d9 
Signed-off-by: Jonah Beckford <[email protected]>
@nojb
Copy link
Collaborator

nojb commented Nov 24, 2022

I'll add in that README a link to https://github.com/diskuv/dkml-workflows#readme (setup-dkml) which a) describes what is hand-generated and what goes into it, and b) includes auto-tested examples and c) implicitly defines a place for bug reports.

Thanks, that's good enough for me! I sync'ed your PR with main and if the CI passes, let's merge.

@nojb nojb merged commit ef58825 into ocaml:main Nov 29, 2022
moyodiallo pushed a commit to moyodiallo/dune that referenced this pull request Dec 2, 2022
@jonahbeckford @anmonteiro
@nojb @moyodiallo
Add CI for MSVC using dkml-workflows (ocaml#6540)
89eb0fe 
Signed-off-by: Jonah Beckford <[email protected]>
Co-authored-by: Antonio Nuno Monteiro <[email protected]>
Co-authored-by: Nicolás Ojeda Bär <[email protected]>
jchavarri added a commit to jchavarri/dune that referenced this pull request Dec 5, 2022
@jchavarri
Merge branch 'main' into melange/non-melange-lib-error
81c1aed 
* main: (54 commits)
  doc: how we write `to_dyn` and `equal` (ocaml#6621)
  test(cache): test output of man pages
  test: dune utop for (subdir ..) (ocaml#6629)
  refactor: improve style of utop rules (ocaml#6628)
  test: depend on utop (ocaml#6627)
  refactor(stdune): Add Appendable_list.cons (ocaml#6624)
  doc: tighten wording in README.md
  test: add a repro case for ocaml#6607 (ocaml#6612)
  doc: cleanup status badges in README.md (ocaml#6618)
  doc(README): rewrap paragraphs and cleanup links
  coq: more resilient config query
  fix: link time code gen (ocaml#6606)
  fix(melange): run melc ppx with merlin (ocaml#6476)
  feature(melange): add compile_flags (ocaml#6569)
  refactor: move `modules: Modules.t` from `Dune_package.Lib` to `Lib_info` (ocaml#6605)
  Set CLICOLOR_FORCE=0 (ocaml#6608)
  fix: declare deps for ccomp detection (ocaml#6610)
  refactor: assume Cmdliner.Arg.conv is abstract (ocaml#6609)
  refactor: gen_rules pattern matching (ocaml#6604)
  Add CI for MSVC using dkml-workflows (ocaml#6540)
  ...
@jonahbeckford jonahbeckford mentioned this pull request Dec 19, 2022
Closed
emillon pushed a commit to emillon/dune that referenced this pull request Dec 20, 2022
@jonahbeckford @anmonteiro
@nojb @emillon
Add CI for MSVC using dkml-workflows (ocaml#6540)
e03c3e0 
Signed-off-by: Jonah Beckford <[email protected]>
Co-authored-by: Antonio Nuno Monteiro <[email protected]>
Co-authored-by: Nicolás Ojeda Bär <[email protected]>
emillon pushed a commit to emillon/dune that referenced this pull request Dec 20, 2022
@jonahbeckford @anmonteiro
@nojb @emillon
Add CI for MSVC using dkml-workflows (ocaml#6540)
c9d593e 
Signed-off-by: Jonah Beckford <[email protected]>
Co-authored-by: Antonio Nuno Monteiro <[email protected]>
Co-authored-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Etienne Millon <[email protected]>
emillon added a commit that referenced this pull request Dec 20, 2022
@emillon @jonahbeckford
@anmonteiro @nojb
Add CI for MSVC using dkml-workflows (#6540) (#6739)
07eff76 
Signed-off-by: Jonah Beckford <[email protected]>
Co-authored-by: Antonio Nuno Monteiro <[email protected]>
Co-authored-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Etienne Millon <[email protected]>

Signed-off-by: Jonah Beckford <[email protected]>
Signed-off-by: Etienne Millon <[email protected]>
Co-authored-by: jonahbeckford <[email protected]>
Co-authored-by: Antonio Nuno Monteiro <[email protected]>
Co-authored-by: Nicolás Ojeda Bär <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anmonteiro anmonteiro anmonteiro left review comments

@nojb nojb nojb approved these changes

Assignees
No one assigned
Labels
windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jonahbeckford @rgrinberg @nojb @anmonteiro