support LDFLAGS in configure and Makefile.config.in except for flexlink#10091
support LDFLAGS in configure and Makefile.config.in except for flexlink#10091gasche merged 1 commit intoocaml:trunkfrom
Conversation
|
Many thanks @gasche.
While you are at it, how about dealing with the LDLIBS variable, too?
In INSTALL.adoc: "Note: some options or variables like may not be taken
into account": if you don't want to deal with LDLIBS then you could
write "like LDLIBS", and if you do take care of LDLIBS too then the
"like" should be removed.
About the definition of `MKEXE_BOOT` in `Makefile.config` (and perhaps
others): I seem to remember that for some linkers it is important that
either LDFLAGS or LDLIBS come last, e.g. after the `-o`. Does that ring
a bell @xavierleroy?
Regarding the comment just above the definition of `mkexe` in
configure.in: some variables are mentionned with a `$` in front of their
name, others are mentionned without it. Perhaps it could be removed from
all the occurrences?
I am wondering about `mksharedlib="$CC -shared $(LDFLAGS)
-flat_namespace`: it seems that, before your patch, this definition did
not make use of `$(OC_LDFLAGS)`. If you add LDFLAGS their, shouldn't
OC_LDFLAGS be added, too?
(the question holds for the other definitions of `mksharedlib`, too.)
With the question above, I realise that we may need to clarify what
`OC_LDFLAGS` represents. To me the intention was for it to contain any
linker flags found to be useful by the configure script and thus LDFLAGS
becomes a mechanism to overload those flags.
|
|
Thanks for the review.
I don't know what LDLIBS is, so I followed that route indeed, thanks for the suggestion.
Yes, this seems to be done in this way in the
I don't really know how OC_LDFLAGS is supposed to work, and I am not sure that the change would be correct. (Given that it is not passed here, I would expect it to be either passed explicitly somewhere else, or ignored. It actually appears to be included in
I am not sure either, but the same question holds for OC_CFLAGS, and I would expect the same answer in both cases. My guess when reading the code was that OC_CFLAGS is meant for settings specifically decided by us for the OCaml build, and CFLAGS is for additional settings decided by the user (I tried to explain this in the comment about |
dra27
left a comment
There was a problem hiding this comment.
You haven't regenerated configure which means in any testing you haven't seen the errors!
|
David Allsopp (2020/12/18 09:53 -0800):
@dra27 commented on this pull request.
You haven't regenerated `configure` which means in any testing you
haven't seen the errors!
Woudl it be possible that Travis checks for this as the pre-commit hook
does?
Will review the rest on Monday.
|
The pre-commit hook doesn't check for this - it's the Travis |
|
Thanks! I hadn't noticed this Travis check. I suppose it was still failing because I regenerated |
|
David Allsopp (2020/12/18 09:53 -0800):
@dra27 commented on this pull request.
You haven't regenerated `configure` which means in any testing you haven't seen the errors!
> @@ -37,11 +37,13 @@ programs_man_section=1
libraries_man_section=3
# Command to build executalbes
-# In general this command is supposed to use the CFLAGs-related variables
-# ($OC_CFLAGS and $CFLAGS), but at the moment they are not taken into
-# account on Windows, because flexlink, which is used to build
-# executables on this platform, can not handle them.
-mkexe="\$(CC) \$(OC_CFLAGS) \$(CFLAGS) \$(OC_LDFLAGS)"
+# In general this command is supposed to use the CFLAGs- and LDFLAGS-
+# related variables (OC_CFLAGS and OC_LDFLAGS for ocaml-specific
+# flags, CFLAGS and LDFLAGS for generic flags chosen by the user), but
+# at the moment they are not taken into account on Windows, because
+# flexlink, which is used to build executables on this platform, can
+# not handle them.
+mkexe="\$(CC) \$(OC_CFLAGS) \$(CFLAGS) \$(OC_LDFLAGS) $(LDFLAGS)"
```suggestion
mkexe="\$(CC) \$(OC_CFLAGS) \$(CFLAGS) \$(OC_LDFLAGS) \$(LDFLAGS)"
```
> @@ -817,7 +819,7 @@ natdynlinkopts=""
AS_IF([test x"$enable_shared" != "xno"],
[AS_CASE([$host],
[*-apple-darwin*],
- [mksharedlib="$CC -shared -flat_namespace -undefined suppress \
+ [mksharedlib="$CC -shared $(LDFLAGS) -flat_namespace -undefined suppress \
This and the other three either need to be:
```suggestion
[mksharedlib="$CC -shared \$(LDFLAGS) -flat_namespace -undefined suppress \
```
or:
```suggestion
[mksharedlib="$CC -shared $LDFLAGS -flat_namespace -undefined suppress \
```
The first one reads `LDFLAGS` when `make` is run, the second reads
`LDFLAGS` at `configure`-time. I'm actually not sure which one
you want for this - the current one means run a command called
`LDFLAGS` which is causing error messages in `configure`.
In any case, I'd say that `$LDFLAGS` should appear as late as possible
in the command so that it gives users a chance to overload any otherwise
set flag. What do you think, @dra27 ?
|
|
Gabriel Scherer (2020/12/18 04:59 -0800):
Thanks for the review.
> if you don't want to deal with LDLIBS then you could write "like LDLIBS"
I don't know what LDLIBS is, so I followed that route indeed, thanks
for the suggestion.
`LDFLAGS`is for flagls like `-L` to specify libraries searchpaths.
`LDLIBS` is to request linking with additional libraries. You would thus
not put `-lfoo` in `LDFLAGS`, but rather in `LDLIBS`.
|
This is a good point, consistent with how CFLAGS is included after OC_CFLAGS and LDFLAGS after OC_LDFLAGS. I rebased the PR so that |
|
You need to include the PR number in the Change entry.
While you are at it, you could use an upper-case letter for the first
letter of the commit'stitle.
In `INSTALL.adoc` I would remove the parentheses aroudn LDLIBS. I do
realise there were parentheses before aroudn LDFLAGS but they actually
make no sense to me so perhaps you could fix that, unless you feel thre
is a good reason to keep them.
|
OC_LDFLAGS is for options specific to linking OCaml programs, LDFLAGS are user-chosen flags that should be included in any call to the system linker. This is intended to fix ocaml#9191. The logic followed in this PR is similar to the treatment of CFLAGS in the build system, which comes from ocaml#9837. Passing LDFLAGS to flexlink is not supported. It is a pain to do correctly, and we haven't got requests from Windows users yet, only from users of unix variants.
|
Thanks, I rebased the PR with all these changes. |
|
Gabriel Scherer (2020/12/21 02:40 -0800):
Thanks, I rebased the PR with all these changes.
Thank you. Re:the parenthesis in `INSTALL.adoc` since it was me who
added this paragraph, I remembered, after writing the previous comment
that the intention was to write `(like LDFLAGS)`.
|
|
Thanks for the approval. I started a |
|
Gabriel Scherer (2020/12/21 05:10 -0800):
Thanks for the approval. I started a `precheck` job and will merge if
the results are good.
Sounds good, thanks. Although we are actually not fully testing,
unfortunately. For that we'd need examples of flags that are really
required, so that not passing them at allo, or not passing them
everywhere where they are needed would result in a failure. I can't
think of such flags, allas.
… |
OC_LDFLAGS is for options specific to linking OCaml programs, LDFLAGS
are user-chosen flags that should be included in any call to the
system linker.
This is intended to fix #9191.
The logic followed in this PR is similar to the treatment of CFLAGS in
the build system, which comes from #9837.
Passing LDFLAGS to flexlink is not supported. It is a pain to do
correctly, and we haven't got requests from Windows users yet, only
from users of unix variants.
(cc our configuration overlords, @shindere and @dra27)