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

Windows/Cygwin: hygienic check fails for some packages #316

Closed
kkirstein opened this issue Feb 16, 2022 · 9 comments · Fixed by #329
Closed

Windows/Cygwin: hygienic check fails for some packages #316

kkirstein opened this issue Feb 16, 2022 · 9 comments · Fixed by #329

Comments

@kkirstein
Copy link

It looks like an old issue has popped up again in ocamlbuild 0.14.1.
For some packages on Windows/Cygwin (Mingw toolchain), the hygienic check fails, e.g., for mtime 1.3.0:

mkdir 'C:\Opt\OCaml64\home\kayuwe.kirstein\.opam\4.13.1+mingw64\.opam-switch\build\mtime.1.3.0\_build'
Das System kann den angegebenen Pfad nicht finden.
''ocamlfind ocamlopt unix.cmxa -I 'C:/Opt/OCaml64/home/kayuwe.kirstein/.opam/4.13.1+mingw64/lib\ocamlbuild' 'C:/Opt/OCaml64/home/kayuwe.kirstein/.opam/4.13.1+mingw64/lib\ocamlbuild/ocamlbuildlib.cmxa' -linkpkg myocamlbuild.ml 'C:/Opt/OCaml64/home/kayuwe.kirstein/.opam/4.13.1+mingw64/lib\ocamlbuild/ocamlbuild.cmx' -o myocamlbuild.exe
SANITIZE: a total of 3 files that should probably not be in your source tree
  has been found. A script shell file
  "C:\\Opt\\OCaml64\\home\\kayuwe.kirstein\\.opam\\4.13.1+mingw64\\.opam-switch\\build\\mtime.1.3.0\\_build/sanitize.sh"
  is being created. Check this script and run it to remove unwanted files or
  use other options (such as defining hygiene exceptions or using the
  -no-hygiene option).
IMPORTANT: I cannot work with leftover compiled files.
ERROR: Leftover object files:
  File myocamlbuild.o in _build has suffix .o
ERROR: Leftover OCaml compilation files:
  File myocamlbuild.cmi in _build has suffix .cmi
  File myocamlbuild.cmx in _build has suffix .cmx
Exiting due to hygiene violations.
pkg.ml: [ERROR] cmd ["ocamlbuild" "-use-ocamlfind" "-classic-display" "-j" "4" "-tag" "debug"
     "-build-dir" "_build" "opam" "pkg/META" "CHANGES.md" "LICENSE.md"
     "README.md" "src/mtime.a" "src/mtime.cmxs" "src/mtime.cmxa"
     "src/mtime.cma" "src/mtime.cmx" "src/mtime.cmi" "src/mtime.mli"
     "src/mtime_top.a" "src/mtime_top.cmxs" "src/mtime_top.cmxa"
     "src/mtime_top.cma" "src/mtime_top.cmx" "src/mtime_top_init.ml"
     "doc/index.mld" "src-os/mtime_clock.a" "src-os/mtime_clock.cmxs"
     "src-os/mtime_clock.cmxa" "src-os/mtime_clock.cma"
     "src-os/mtime_clock.cmx" "src-os/mtime_clock.cmi"
     "src-os/mtime_clock.mli" "src-os/dllmtime_clock_stubs.dll"
     "src-os/libmtime_clock_stubs.a" "test-os/min_os.ml"]: exited with 1

I am not sure whether the first error message on not finding the build folder is a serious failure.
Executing sanitize.sh does not help, I guess opam install mtime regenerates the object files every time.

Observed on Windows 10, Cygwin/Mingw (Ocaml for Windows) for OCaml 4.13.1+mingw64.
My current workaround is downgrading ocamlbuild to 0.14.0.

@kit-ty-kate
Copy link
Member

opam-repository-mingw has a patch for ocamlbuild apparently.
I have no idea what it’s supposed to fix but could you give it try to see if it fixes your issue?

https://github.com/fdopen/opam-repository-mingw/blob/df5006bfe42608c42d02ad1f23a44f2451f35e3a/packages/ocamlbuild/ocamlbuild.0.14.0/files/ocamlbuild-0.14.0.patch

@kkirstein
Copy link
Author

@kit-ty-kate You pointed to the right location. Actually I had a mix-up of different versions.
I have both repos upstream opam-repository from ocaml.org & opam-repository-mingw from github/fdopen in my opam setup (This is the suggested configuration, as the fdopen repo is deprecated for ordinary packages). I thought opam always takes the packages from the first repo, if available, but it seems to give priority to the highest patchlevel.

Anyway, here is the (corrected) list of ocamlbuild version & observed issue:

  • 0.14.0-patch (opam-repository-mingw): works
  • 0.14.0 (opam-repository): observed hygienic issue
  • 0.14.1 (opam-repository): observed hygienic issue

So, it looks like the patch from opam-repository-mingw is needed for Windows/Cygwin. It would be nice to have that merged into the ocamlbuild sources. Are there any reasons not to do this? If yes, I would suggest to add a constraint to ocamlbuild.opam like os != "win32" & os-distribution != "cygwinports", so one can not accidently pick up an unpatched version.

@gasche
Copy link
Member

gasche commented Feb 17, 2022

Are there any reasons not to do this?

To my knowledge, no one has done the work of submitting the patch upstream (here) yet, and (hopefully) explaining what it does.

@gasche
Copy link
Member

gasche commented Feb 17, 2022

@kkirstein if you were interested in getting some of this patch merged, I think that would be very nice! There are some parts that were not intended for upstreaming (they are redundant with other code, etc., not written with maintenability in mind), some parts that could be simplified, some parts whose purpose/effect is unclear, etc. (And of course no tests :-) I think someone would need to split it into separate commits that make sense individually (instead of mixing unrelated things together), and submit PRs. I'm happy to help by reviewing the PRs then, but given that I don't have a Windows machine I cannot help with the testing.

@gasche
Copy link
Member

gasche commented Feb 17, 2022

Note: another route for you would be of course to submit an ocamlbuild.14.1 package in opam-repository-mingw, with the same patch as before (I would guess that the rebase would be very easy). That would not help for future versions of reduce the maintenance burden, but it's certainly a good short-term step you could take, hopefully in parallel to my suggestion above.

(For the record, the OCaml Foundation offered funding to fdopen, the author and maintainer of opam-repository-mingw, to keep maintaining the patches and work on upstreaming them in their respective projects, but fdopen's opinion at the time (in addition to a lack of time available to work on this) was that it's less necessary nowadays because more recent packages, in particular those using dune, work fine under Windows, so the repository is getting less and less relevant. That sounds like a reasonable position to me, but I'm not familiar with the Windows situation.)

@kkirstein
Copy link
Author

@gasche I would agree with fdopen that we should keep the number of packages in the mingw repo with special patches small and concentrate of merging Windows-related patches to the upstream repos.
I had a (very rough) look at the patch and indeed there seems to quite some stuff in it, where some of it probably is not needed anymore or unrelated to the observed issue. But (at least for me) it is hard to sort that out.
In addition, we have to be careful not to break the other target systems, like "pure" Cygwin and "pure" Windows.

Anyway, I will have a closer look at fixing the hygienic/path issue, but I am not sure I can provide something in the near future.

@kkirstein
Copy link
Author

A more general question: What is the roadmap for ocamlbuild anyway? dune is getting more and more adoption and has advantage with respect to platform independence. But I also remember some posts on discuss.ocaml.org of people who want to stick with ocamlbuild.

@gasche
Copy link
Member

gasche commented Feb 17, 2022

Right now ocamlbuild is in maintenance-mode only, with no new developments planned, and the idea is to encourage people to migrate to dune. We keep the lights on and write the easy fixes and merge PRs when they look good, but not much more.

@dbuenzli
Copy link
Collaborator

So far I still use it in my packages to build my distributions via topkg. Thanks for keeping the light on !

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 a pull request may close this issue.

4 participants