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

fix: re-enable dune on older macos sdk's #6515

Merged
merged 1 commit into from Nov 24, 2022

Conversation

rgrinberg
Copy link
Member

by disabling watch mode on such versions

Signed-off-by: Rudi Grinberg me@rgrinberg.com

ps-id: dc8e96cc-61a9-4497-baac-0934b82efe6f

@rgrinberg
Copy link
Member Author

@barracuda156 can you check if this works?

@barracuda156
Copy link

@barracuda156 can you check if this works?

Sure, gimme few min.

@barracuda156
Copy link

On a side note, and since the PR refers to SDKs, there is one more issue that has to be fixed for those: #6510 (large chunk of those functions in fsevents_stubs are 10.7+).

That one I can fix locally for myself, but it is perhaps desirable to fix it for everyone. I could have made a PR, but I hope that maybe someone can come up with a better fix than merely disabling the functions – like it was done for libuv: libuv/libuv@6602fca

@rgrinberg
Copy link
Member Author

I tried making it work for < 10.7 but it seems to be difficult. There's no notification for file level notifications so it doesn't really work well for dune's needs.

@rgrinberg rgrinberg added this to the 3.7.0 milestone Nov 19, 2022
@barracuda156
Copy link

I tried making it work for < 10.7 but it seems to be difficult. There's no notification for file level notifications so it doesn't really work well for dune's needs.

Original fix was a blank disabling: #5431
That would actually work for <10.7 (with obvious limitations), but this is perhaps a feasible and surely better option: https://en.wikipedia.org/wiki/Kqueue

@rgrinberg
Copy link
Member Author

Kqueue isn't really better. The API is much lower level and incredibly inconvenient for watching entire trees recursively. We deal with a similar headache on inotify though, so it is feasible.

@barracuda156
Copy link

Kqueue isn't really better. The API is much lower level and incredibly inconvenient for watching entire trees recursively. We deal with a similar headache on inotify though, so it is feasible.

In Macports at least, ocaml-dune is a necessity to install a number of ports (only one build method can be supported per port), so even if some valuable functionality is not available on older OS, as long as it works to build packages, that is infinitely better than being just broken :)

(For a context, I am trying now to fix Stan for PPC and make the whole family of its ports available in Macports. Turned out, upstream uses OCaml interpreter to build models, and that interpreter depends upon a gazillion of tiny ports which all need Dune to build.)

@barracuda156
Copy link

@rgrinberg I have just now looked at the files changed preparing to build, and my assumption was wrong: you actually addressed the issue which I thought was pending.
However, this is broken: #6156
Unfortunately, I have no solution for that one, and while the failure looks trivial, I could not fix it (primarily because I cannot understand why it fails in the first place).

@rgrinberg
Copy link
Member Author

Can you explain how to reproduce the error?

@barracuda156
Copy link

barracuda156 commented Nov 19, 2022

Can you explain how to reproduce the error?

I did not do anything special to get it. Whatever I tried to change, it persisted.

If versions are important:
ocamlc 4.14.0 (bytecode compiler)
ocaml-dune 3.6.0 / latest commit (same failure)
ocaml-csexp 1.5.1

This is not the case where I would tweak anything in an exotic manner :)

OCaml packages normally just build (including for Darwin PPC) with zero extra effort, the only thing I modify* as compared to Macports master, as far as OCaml goes, is updating to current versions (Macports is lazy or reluctant to update those).
I have tried at least three recent versions of ocaml-dune. None of those could build ocaml-dune-configurator (failing identically, unable to locate ocaml-csexp) , which I do require functional for a said reason (moving dozens of packages to another build system would be insane and cannot be portable to others).

I think Macports logic with these ports is simple and transparent. ocaml PG sets directories and flags, portfiles borrow those, everything works, aside of one instance.
As I mentioned, ocaml-csexp is not only installed de facto but also visible to external environment. Why ocaml-dune-configurator fails to find it is beyond me.

P. S. * Well, irrelevant to this discussion and packages in question, but for the sake of being precise, few OCaml packages do need a correct target being set manually in order ocamlopt not to be forced on PPC.

@barracuda156
Copy link

@rgrinberg Since I could not find a rational solution to the problem which needs to be solved somehow, I also rebuilt from scratch all three in question: OCaml itself, ocaml-dune, ocaml-csexp.

P. S. Since ocaml-findlib can see ocaml-csexp as installed without any issues, it is likely possible to manually re-write the whole build & install process without Dune, using ocamlc and ocamlbuild, but this is not something which I can sit and do without understanding two alien build systems well enough.

@barracuda156
Copy link

@rgrinberg Okay, I have fixed it for myself. Apparently sources are inconsistent: for w/e reason included csexp package is deleted by scripts, while the library is defined as local one. It was easier for me to fix usage of included csexp by restoring .opam file and patching few files accordingly. Now everything started working, finally.

I can test your SDK fix now too.

@barracuda156
Copy link

barracuda156 commented Nov 21, 2022

@rgrinberg Your patch works fine, the only thing, I believe, macro should be used after the header, as it is always done (otherwise you put the header inside the conditional which is determined by the header in question, that might work, but should not). I.e.:

--- src/fsevents/fsevents_stubs.c.orig	2022-11-14 22:11:03.000000000 +0800
+++ src/fsevents/fsevents_stubs.c	2022-11-21 12:46:25.000000000 +0800
@@ -7,8 +7,10 @@
 #include <caml/threads.h>
 
 #if defined(__APPLE__)
+#include <AvailabilityMacros.h>
+#endif
 
-#include <Availability.h>
+#if __MAC_OS_X_VERSION_MAX_ALLOWED >= 100700
 #include <CoreFoundation/CoreFoundation.h>
 #include <CoreServices/CoreServices.h>

(Wrapping is inconsequential, #endif can go to the end of file instead.)

P. S. Line 93 change does nothing or I miss something? It is inside a macro for new systems, so I cannot check it.

@rgrinberg rgrinberg force-pushed the ps/rr/fix__re_enable_dune_on_older_macos_sdk_s branch from 28a929d to 3bd1151 Compare November 21, 2022 23:56
by disabling watch mode on such versions

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

ps-id: dc8e96cc-61a9-4497-baac-0934b82efe6f
@rgrinberg rgrinberg force-pushed the ps/rr/fix__re_enable_dune_on_older_macos_sdk_s branch from 3bd1151 to 92c610f Compare November 21, 2022 23:58
@rgrinberg
Copy link
Member Author

@rgrinberg Your patch works fine, the only thing, I believe, macro should be used after the header, as it is always done (otherwise you put the header inside the conditional which is determined by the header in question, that might work, but should not). I.e.:

--- src/fsevents/fsevents_stubs.c.orig	2022-11-14 22:11:03.000000000 +0800
+++ src/fsevents/fsevents_stubs.c	2022-11-21 12:46:25.000000000 +0800
@@ -7,8 +7,10 @@
 #include <caml/threads.h>
 
 #if defined(__APPLE__)
+#include <AvailabilityMacros.h>
+#endif
 
-#include <Availability.h>
+#if __MAC_OS_X_VERSION_MAX_ALLOWED >= 100700
 #include <CoreFoundation/CoreFoundation.h>
 #include <CoreServices/CoreServices.h>

(Wrapping is inconsequential, #endif can go to the end of file instead.)

P. S. Line 93 change does nothing or I miss something? It is inside a macro for new systems, so I cannot check it.

Thanks. I applied your suggestion.

@rgrinberg rgrinberg merged commit fdd5cd4 into main Nov 24, 2022
jchavarri added a commit to jchavarri/dune that referenced this pull request Nov 24, 2022
* main: (58 commits)
  test: formatting of alternative dune files (ocaml#6567)
  refactor: remove Modules.is_empty (ocaml#6564)
  refactor: module kinds (ocaml#6562)
  refactor(coq): resolve lack of coqc properly
  Cache file contents in action builder by name. (ocaml#6555)
  fix: re-enable dune on older macos sdk's (ocaml#6515)
  fix: do not hide lib interface module (ocaml#6549)
  test: remove pkg-config output for reproducibility (ocaml#6543)
  melange: add test for ocaml flags (ocaml#6548)
  fix: improve virtual library error messages
  test: virtual library and impl locations
  test: alias module regression (ocaml#6544)
  refactor(merlin): dump config sub command (ocaml#6547)
  refactor: simplify merlin (ocaml#6508)
  chore(nix): use nix-overlays for the slim devShell (ocaml#6546)
  fix: module compilation rule env (ocaml#6527)
  chore: update nix (ocaml#6536)
  fix: merlin rules with pp's (ocaml#6474)
  Call [Dune_util.Log.init] as soon as possible (ocaml#6542)
  refactor: speed up stdlib build (ocaml#6524)
  ...
jchavarri added a commit to jchavarri/dune that referenced this pull request Nov 24, 2022
* main:
  test: formatting of alternative dune files (ocaml#6567)
  refactor: remove Modules.is_empty (ocaml#6564)
  refactor: module kinds (ocaml#6562)
  refactor(coq): resolve lack of coqc properly
  Cache file contents in action builder by name. (ocaml#6555)
  fix: re-enable dune on older macos sdk's (ocaml#6515)
  fix: do not hide lib interface module (ocaml#6549)
  test: remove pkg-config output for reproducibility (ocaml#6543)
  melange: add test for ocaml flags (ocaml#6548)
  fix: improve virtual library error messages
  test: virtual library and impl locations
  test: alias module regression (ocaml#6544)
  refactor(merlin): dump config sub command (ocaml#6547)
@Alizter Alizter deleted the ps/rr/fix__re_enable_dune_on_older_macos_sdk_s branch November 26, 2022 14:46
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.

fsevents_stubs uses functions unavailable on older macOS which breaks the build
3 participants