-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Build system: make it possible to choose which ocamldep (and flags) to use #11126
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One important change, and a couple of suggestions, but this looks nice to me! I've also verified locally that make alldepend
still works (with the correction to stdlib/Makefile
).
Given that the flags are added, perhaps this is a good point to add -no-slash
to ocamldep (so that one can cancel the option using OCAMLDEPFLAGS
):
diff --git a/driver/compenv.ml b/driver/compenv.ml
index 5e247c9941..ce74352e56 100644
--- a/driver/compenv.ml
+++ b/driver/compenv.ml
@@ -260,6 +260,7 @@ let read_one_param ppf position name v =
| "verbose" -> set "verbose" [ verbose ] v
| "nopervasives" -> set "nopervasives" [ nopervasives ] v
| "slash" -> set "slash" [ force_slash ] v (* for ocamldep *)
+ | "no-slash" -> clear "no-slash" [ force_slash ] v (* for ocamldep *)
| "keep-docs" -> set "keep-docs" [ Clflags.keep_docs ] v
| "keep-locs" -> set "keep-locs" [ Clflags.keep_locs ] v
diff --git a/driver/makedepend.ml b/driver/makedepend.ml
index c05abe22ef..ff332f4b72 100644
--- a/driver/makedepend.ml
+++ b/driver/makedepend.ml
@@ -637,6 +637,8 @@ let run_main argv =
" Generate dependencies for native plugin files (.cmxs targets)";
"-slash", Arg.Set Clflags.force_slash,
" (Windows) Use forward slash / instead of backslash \\ in file paths";
+ "-no-slash", Arg.Clear Clflags.force_slash,
+ " (Windows) Preserve any backslash \\ in file paths";
"-sort", Arg.Set sort_files,
" Sort files according to their dependencies";
"-version", Arg.Unit print_version,
Makefile.menhir
Outdated
# This rule depends on the OCAMLDEP, OC_OCAMLDEPFLAGS and OC_OCAMLDEPDIRS | ||
# variables defined in Makefile, so it can only be invoked from the | ||
# main Makefile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# This rule depends on the OCAMLDEP, OC_OCAMLDEPFLAGS and OC_OCAMLDEPDIRS | |
# variables defined in Makefile, so it can only be invoked from the | |
# main Makefile | |
# This rule depends on the OCAMLDEP_CMD variable defined in Makefile.common, | |
# so it can only be invoked from the main Makefile |
ocamldoc/Makefile
Outdated
|
||
INCLUDE_DIRS = $(addprefix $(ROOTDIR)/,\ | ||
utils parsing typing driver bytecomp toplevel) | ||
INCLUDES_DEP = $(addprefix -I ,$(INCLUDE_DIRS)) | ||
INCLUDES_NODEP=\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this one be refactored with $(addprefix
to be consistent with the INCLUDES_DEP
? So
INCLUDES_NODEP = $(addprefix -I $(ROOTDIR)/,\
stdlib compilerlibs otherlibs/str otherlibs/dynlink otherlibs/dynlink/native otherlibs/$(UNIXLIB))
stdlib/Makefile
Outdated
# NOTE: it is important that the OCAMLDEP variable is defined *before* | ||
# Makefile.common gets included, so that its local definition here | ||
# take precedence over its general shared definitions in Makefile.common. | ||
OCAMLDEP ?= $(BOOT_OCAMLCDEP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be BOOT_OCAMLDEP
(there's a stray capital C
before DEP
)
@@ -262,14 +264,18 @@ GNUISH_SED = \ | |||
$(if $(filter X,$(shell echo x | $(SED) -E -e 's/./\u&/' 2>/dev/null)),\ | |||
$(SED),$(error GNU sed is needed for make depend)) | |||
|
|||
.INTERMEDIATE: .depend.tmp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Many thanks for the detailed feedback, @dra27.
Once again it has been sad to notice how the text version of the e-mails
sent by GitHub is broken, compared to the HTML one. Would really be nice
to get this acted upon.
David Allsopp (2022/03/22 09:21 -0700):
@dra27 commented on this pull request.
One important change, and a couple of suggestions, but this looks nice to
me! I've also verified locally that make alldepend still works (with the
correction to stdlib/Makefile).
I did check but missed that. Thanks.
Given that the flags are added, perhaps this is a good point to add
-no-slash to ocamldep (so that one can cancel the option using
OCAMLDEPFLAGS):
[...]
Yes! Good idea, thanks.
I added a separate commit for that and credited you for the change in
the commit message.
In [1]Makefile.menhir:
> +# This rule depends on the OCAMLDEP, OC_OCAMLDEPFLAGS and OC_OCAMLDEPDIRS
+# variables defined in Makefile, so it can only be invoked from the
+# main Makefile
� Suggested change
-# This rule depends on the OCAMLDEP, OC_OCAMLDEPFLAGS and OC_OCAMLDEPDIRS
-# variables defined in Makefile, so it can only be invoked from the
-# main Makefile
+# This rule depends on the OCAMLDEP_CMD variable defined in Makefile.common,
+# so it can only be invoked from the main Makefile
Fixed. I also clarified the comment a bit and moved it above the rule it
talks about.
══════════════════════════════════════════════════════════════════════════
In [2]ocamldoc/Makefile:
>
+INCLUDE_DIRS = $(addprefix $(ROOTDIR)/,\
+ utils parsing typing driver bytecomp toplevel)
+INCLUDES_DEP = $(addprefix -I ,$(INCLUDE_DIRS))
INCLUDES_NODEP=\
Can this one be refactored with $(addprefix to be consistent with the
INCLUDES_DEP? So
INCLUDES_NODEP = $(addprefix -I $(ROOTDIR)/,\
stdlib compilerlibs otherlibs/str otherlibs/dynlink
otherlibs/dynlink/native otherlibs/$(UNIXLIB))
Done. Thanks.
══════════════════════════════════════════════════════════════════════════
In [3]stdlib/Makefile:
> @@ -14,6 +14,10 @@
#**************************************************************************
ROOTDIR = ..
+# NOTE: it is important that the OCAMLDEP variable is defined *before*
+# Makefile.common gets included, so that its local definition here
+# take precedence over its general shared definitions in Makefile.common.
+OCAMLDEP ?= $(BOOT_OCAMLCDEP)
This should be BOOT_OCAMLDEP (there's a stray capital C before DEP)
Absolutely. Thanks. Fixed.
|
Changes
Outdated
@@ -189,6 +189,10 @@ Working version | |||
- #11092: Build native-code compilers on OpenBSD/aarch64. | |||
(Christopher Zimmermann, review by Anil Madhavapeddy) | |||
|
|||
- #11126: Build system: make it possible to choose which ocamldep | |||
(and flags) to use when computiing depencies for the compiler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and flags) to use when computiing depencies for the compiler. | |
(and flags) to use when computing depencies for the compiler. |
This is so that a user can override a -slash option. This change has been kindly contributed by David Allsopp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to go to me, thanks - @Octachron, what is the glaciated state of trunk (vis-à-vis the addition of -no-slash
to ocamldep?)
David Allsopp (2022/03/28 02:59 -0700):
@dra27 approved this pull request.
This looks good to go to me, thanks - @Octachron, what is the glaciated state of trunk (vis-à-vis the addition of `-no-slash` to ocamldep?)
Many thanks for the approval!
|
Concerning the freeze, I would propose to circumvent the issue: |
Aside, my understanding of the sequential glaciation was that it was over when multicore got merged. Hence I recently pinged a few stdlib PRs, sorry about that. |
That was my understanding too.
My belief was/is is that trunk is not frozen now, although we perhaps
try to keep the set of new features minimal as far as the language and
runtime are ocncerned.
|
My understanding is that trunk will be frozen until the moment that the 5.0 branch is cut. I am aiming to cut the branch as soon as the remaining issues in #11013 are resolved. |
Florian Angeletti (2022/03/29 11:43 +0000):
My understanding is that trunk will be frozen until the moment that
the 5.0 branch is cut. I am aiming to cut the branch as soon as the
remaining issues in #11013 are resolved.
Ah, okay. I got a few PRs merged but perhaps because they fixed
something.
So can we merge this one or should it wait?
|
Let's merge, like I said this is mostly a new ocamldep feature (and arguably a fix) and not a change to the compiler itself. |
Florian Angeletti (2022/03/29 06:52 -0700):
Let's merge, like I said this is mostly a new ocamldep feature (and
arguably a fix) and not a change to the compiler itself.
Thanks. Done so, as can be seen.
|
sabine (2022/03/23 01:33 -0700):
@sabine commented on this pull request.
> @@ -189,6 +189,10 @@ Working version
- #11092: Build native-code compilers on OpenBSD/aarch64.
(Christopher Zimmermann, review by Anil Madhavapeddy)
+- #11126: Build system: make it possible to choose which ocamldep
+ (and flags) to use when computiing depencies for the compiler.
```suggestion
(and flags) to use when computing depencies for the compiler.
```
Many thanks for your attention, @sabine. Fixed and added a mention of
the -no-slash option to the Changes entry.
|
This PR standardizes the invocations of ocamldep.
It is now possilbe to choose which ocmaldep is used to compute dependencies
like this:
Independently of which ocamldep is called, it is now also possible to
specify additional flags which will be passed to the invoked ocamldep in
adition to those passed by the build system (currently only
-slash
). It isguaranteed that the additional flags will really be passed in addition to
the build system defined ones (rather than replacing them). It is also
guaranteed that the behaviour will be consistent accross all the
invocations of ocamldep.
The more general context of this PR is to make it possible and easier to use
the user-chosen tools during the build of the compiler, rather than having to
rely on in-tree tools because this behaviour is hard-coded in the build
system itself.
It is already possible to choose the runtime one wants to use and
one possible next step is to be able to use an installed version of
ocamlc
as a bootstrap compiler, rather than having to rely on
boot/ocamlc
.Similarly, one may want to be able to easily add user-chosen
compiler flags to those defined by the build system when compiling
the compiler, for instance to build an instrumented version of
the compiler itself.
Also, this PR fixes the
Makefile
of the memory model test which wasincorrectly invoking
ocamldep
directly, assuming there is alreadyan installed one available, which is not necessarily the case -- @maranget
may be interested in having a look.