Skip to content

Various changes for building with dune #674

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

Merged
merged 9 commits into from
Mar 12, 2022
Merged

Various changes for building with dune #674

merged 9 commits into from
Mar 12, 2022

Conversation

olafhering
Copy link
Contributor

This is a followup for issue #658.

It addresses all the comments.

In addition it provides an attempt to build unison-fsmonitor with dune on Solaris.

The hacks used to build on OCaml < 4.08 are only applied via makefiles.
Adjust the minimal required version to 4.08.

This change has an effect only if generate_opam_files is set to true.

Signed-off-by: Olaf Hering <[email protected]>
The ocamlfind name is lablgtk2, but the opam package name is just lablgtk.

Also remove the specific version for lablgtk2. Let opam decide which
version is compatible with the base OCaml version.

This change has an effect only if generate_opam_files is set to true.

Signed-off-by: Olaf Hering <[email protected]>
This is also done in alcotest.git/dune-project.
This change has an effect only if generate_opam_files is set to true.

Signed-off-by: Olaf Hering <[email protected]>
The dune documentation states: authors is a list of items. Convert
the single string into a list of space separated strings.

This change has an effect only if generate_opam_files is set to true.

Signed-off-by: Olaf Hering <[email protected]>
This change has an effect only if generate_opam_files is set to true.

Signed-off-by: Olaf Hering <[email protected]>
This change has an effect only if generate_opam_files is set to true.

Signed-off-by: Olaf Hering <[email protected]>
Preserve the status quo and reuse the maintainer listed currently in
the opam repository, and in the local copy of the opam file

This change has an effect only if generate_opam_files is set to true.

Signed-off-by: Olaf Hering <[email protected]>
Each library needs to have an unique name, even if they are
constrained with enabled_if. Once fsmonitor is build with dune for
other targets, dune will complain about duplicate names. Avoid this
error by giving the internal library a more specific name.

Signed-off-by: Olaf Hering <[email protected]>
Untested change, copy&paste from the linux file.

Signed-off-by: Olaf Hering <[email protected]>
@gdt
Copy link
Collaborator

gdt commented Mar 9, 2022

Does this supercede #671?
Once the relationship of the two PRs is resolved, I likely will merge something immediately after 2.52.0, which I am hoping will be unchanged from 2.51.91.

@olafhering
Copy link
Contributor Author

I was not aware of the other PR, and left a comment over there.

@gdt
Copy link
Collaborator

gdt commented Mar 12, 2022

I've reviewed both, and I intend to hit merge on this PR immediately after release., and let @liyishuai see if there is anything to change in #671 not done by #674. I don't think it's risky, but I just don't like to make changes between rc and release other than version number.

Thanks for having logically separate commits - that made it much easier/faster to understand. (I am not yet 100% clear on the wisdom of generating opam files vs checked in; happy to consider but I would prefer any such change to be separated from any other changes that it can be separated from.)

@tleedjarv
Copy link
Contributor

I managed to get dune working enough to briefly test af57181. It seems to work.

I also tried merging library and executable and that worked for me as well. I don't know if you should include it or not (you know dune by far more than I do), but here's how I did it:

diff --git a/src/fsmonitor/solaris/dune b/src/fsmonitor/solaris/dune
index 1f5abf7..02bde4a 100644
--- a/src/fsmonitor/solaris/dune
+++ b/src/fsmonitor/solaris/dune
@@ -1,22 +1,12 @@
 (copy_files# ../watchercommon.ml{,i})
 
-(library
- (name fswatcher_solaris)
- (wrapped false)
- (enabled_if (= %{system} "solaris"))
- (modules :standard \ watcher)
- (flags :standard -w -3-27-39)
- (foreign_stubs
-  (language c)
-  (names fen_stubs))
- (libraries unix lwt_lib))
-
 (executable
  (name watcher)
  (public_name unison-fsmonitor)
  (package unison-fsmonitor)
  (enabled_if (= %{system} "solaris"))
- (modules watcher)
+ (foreign_stubs
+  (language c)
+  (names fen_stubs))
  (flags :standard -w -27)
- (libraries fswatcher_solaris))
+ (libraries unix lwt_lib))

(I also got rid of -w -39 as you can see, but that requires a fix in watchercommon.ml; see #676)

@gdt gdt merged commit 1a23674 into bcpierce00:master Mar 12, 2022
@olafhering olafhering deleted the dune branch March 13, 2022 10:32
@olafhering
Copy link
Contributor Author

Thanks for testing the solaris dune file. For me this variant works, no idea why each module needs to be specified. Feel free to merge it, I have no idea how to do runtime testing.

@@ -28,6 +11,9 @@
    (= %{system} "elf")
    (= %{system} "linux_eabihf")
    (= %{system} "linux_eabi")))
- (modules watcher)
+ (modules inotify lwt_inotify watcher watchercommon)
  (flags :standard -w -27)
- (libraries fswatcher_linux))
+ (foreign_stubs
+  (language c)
+  (names inotify_stubs))
+ (libraries unix lwt_lib))

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.

3 participants