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

Work with fsevents/cf libs #31

Merged
merged 2 commits into from Apr 26, 2021
Merged

Work with fsevents/cf libs #31

merged 2 commits into from Apr 26, 2021

Conversation

avsm
Copy link
Member

@avsm avsm commented Apr 25, 2021

Supersedes #23

Copy link
Member

@samoht samoht left a comment

Choose a reason for hiding this comment

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

LGTM apart minor tweaks

Thanks for fixing this!

CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
"inotify" {os = "linux"}
"osx-fsevents" {os = "macos" & >= "0.2.0"}
"inotify" {os = "linux"}
"cf-lwt"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"cf-lwt"
"cf-lwt" {os = "macos"}

Copy link
Member Author

Choose a reason for hiding this comment

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

actually not needed any more -- I modified the dependent libraries to be optional but installable on Linux, so it'll work in a duniverse. So they can be firm dependencies now.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to not force these dependencies if possible (to avoid future compilation issues on Linux due to unrelated issues on macos).

Copy link
Member Author

Choose a reason for hiding this comment

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

we need it to work in a duniverse though. The non-macOS builds don't do anything on Linux at all -- i used enabled_if to just not compile them when system <> macOS. https://github.com/mirage/ocaml-cf/blob/main/lib/dune#L7

So the whole dependency should be a noop if os != macos (but it will jsut work if irmin-watcher is cloned into a duniverse and compiled on a mac)

"osx-fsevents" {os = "macos" & >= "0.2.0"}
"inotify" {os = "linux"}
"cf-lwt"
"fsevents-lwt"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"fsevents-lwt"
"fsevents-lwt" {os = "macos"}

src/polling.ml Outdated Show resolved Hide resolved
@samoht
Copy link
Member

samoht commented Apr 26, 2021

I'm merging this one so that we can unlock the build in a dune monorepo.

@samoht samoht merged commit 753084b into mirage:master Apr 26, 2021
avsm added a commit to avsm/opam-repository that referenced this pull request May 4, 2021
CHANGES:

- Switch to GitHub Actions from Travis (mirage/irmin-watcher#31, @avsm)
- Initialise backends only when needed via a
  lazy watcher interface (mirage/irmin-watcher#31, @samoht @avsm)
- Use fsevents and cf-lwt packages (mirage/irmin-watcher#31, @avsm)
- Use ocamlformat.0.18.0 (mirage/irmin-watcher#31, @avsm)
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.

None yet

2 participants