-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Relocatable OCaml - explicit-relative paths in ld.conf
#14243
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
base: trunk
Are you sure you want to change the base?
Conversation
| extern char_os * caml_parse_ld_conf(void); | ||
| /* If found, parse $OCAMLLIB/ld.conf, $CAMLLIB/ld.conf and stdlib/ld.conf in | ||
| that order and add the lines read to table. */ | ||
| extern char_os * caml_parse_ld_conf(const char_os * stdlib, |
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.
FYI: I did a GitHub search for caml_parse_ld_conf since adding arguments to the argument list of caml_parse_ld_conf is veering into C undefined behavior (if client code accesses an old caml_parse_ld_conf in shared library, or worse if vice versa). No one outside of startup_byt.c is using it, so it looks safe to change.
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.
Indeed, it's guarded by CAML_INTERNALS as well (it's actually only been exposed at all since #9284 in 4.13.0 in order to implement ocamlrun -config ... which was very very early work on Relocatable!)
| { | ||
| const char_os * stdlib; | ||
| stdlib = caml_secure_getenv(T("OCAMLLIB")); | ||
| if (stdlib == NULL) stdlib = caml_secure_getenv(T("CAMLLIB")); |
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.
For portable consistency, can we check for NULLs and empty strings? (Unless it is documented somewhere that empty OCAMLLIB stops search for CAMLLIB and OCAML_STDLIB_DIR, which would be weird).
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.
I've tackled this in #14247 (the specific bit can be previewed in dra27#227, although this function moves about a bit, so it's in dynlink.c in that PR still!). My rationale was to preserve the "status quo" for in this PR, just because it's not strictly necessary to fix it but, as you can see from the follow-up (and from dra27#225), I firmly agree that we should treat NULL and empty as equivalent!
|
LGTM, with minor comment. I didn't review the test suite since I'm unfamiliar with that. |
|
Thanks, @jonahbeckford!
Some more detail for these changes. They're slightly better looked at commit-by-commit:
|
|
Surfacing a brief discussion on caml-devel about a design-decision affecting this. Non-blank lines in
In OCaml today, only absolute directories have a sensible semantics, so the core idea in this PR is to add a sensible semantics to either or both of the other two lines (explicit-relative, implicit), on the basis that they're presently not in use. A complete test for an absolute path is quite difficult to get right on Windows, which is necessarily needed to determine if a line is an implicit path. However, the explicit-relative test is straightforward, which is the main reason I opted just to interpret explicit-relative directories with this new mechanism (it is also the case that opam-bin uses a In this PR right now, we have explicit-relative paths interpreted relative to where There are two changes either or both of which could be made. Firstly, the paths could be instead be interpreted relative to The second change would be to recognise both explicit-relative and implicit in the same way, allowing The decision in this PR has a small bearing on but which could therefore be: |
bytecomp/dll.ml
Outdated
| if line = "" then | ||
| "" | ||
| else | ||
| let len = String.length line in |
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.
I've tried to make the logic easier to understand. I hope I've managed to preserve all aspect.
if Filename.is_relative line && not (Filename.is_implicit line)
then
match line with
| "." -> Config.standard_library
| _ -> Filename.concat Config.standard_library line
else lineThere 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.
(note that this function gets completely deleted in 1ddfabc, where Dll co-opts the C implementation).
It's different for line = "./stublibs", I think - the previous version (and the version in C) produces <libdir>/stublibs, but the newer version produces <libdir>/./stublibs. It's pedantic, but I was trying where possible to avoid the stray /./ bits being introduced, given that the default entries are . and ./stublibs!
Thanks, that was helpful for me to review the testsuite. It all looks good to me. |
3982953 to
e0f412e
Compare
|
Rebased - and with one additional change inspired by #14244 (comment). I've renamed |
|
Another follow-up from #14246 (comment). The last commit in this series corrected the over-installation of the native stub libraries added in #11828. That's only a partial fix (they shouldn't be built either) and it doesn't belong in this PR. |
--with-stublibs is intended for use in opam and allows an additional directory to be added at the top of $(ocamlc -where)/ld.conf.
- Rename --with-stublibs to --with-additional-stublibsdir (more closely matches the variable which was being used internally, apart from anything else) - As with --with-relative-libdir, have a sensible default value for --with-additional-stublibsdir, corresponding to the opam/findlib interpretation
When calculating the full path for ld.conf, the runtime unconditionally
concatenated "/ld.conf". This is harmless when the separators appear in
the middle of a path ("/usr/local/lib/ocaml//ld.conf" is equivalent to
the version with only single slashes), but it is technically incorrect
for two corners cases with OCAMLLIB and CAMLLIB:
- if either is explicitly set to "/" then "//ld.conf" is _not_ the same
file as "/ld.conf". This is mildly relevant on Windows and Cygwin
where the two initial slashes (including as "\/" for native Windows)
will be interpreted as a UNC path
- if either is explicitly blank, then "ld.conf" (i.e. ld.conf in the
current directory) is a less illogical file to open than "/ld.conf"
Explicit relative paths in ld.conf are now interpreted relative to the directory containing ld.conf.
Before, the first ld.conf found from $OCAMLLIB, $CAMLLIB or the preconfigured standard library location was loaded. Now all of these are loaded.
The function was only ever added to share the logic between dynlink.c and startup_byt.c - now that dynlink.c doesn't require it, move the function to startup_byt.c and make it internal again.
Eliminate the need for two implementations of the parsing logic for ld.conf by sharing the C implementation (which must exist, since it's part of bytecode startup) with the bytecode compiler, replacing Dll.ld_conf_contents
- $libdir/stublibs is no longer created for a --disable-shared build - When $libdir/stublibs is not created, it is also not added to ld.conf
This is the first of three PRs which implement Relocatable OCaml as proposed in ocaml/RFCs#53. This PR specifically deals with changes to the handling of
ld.conf.The key changes are:
configureoption--with-stublibsadded to address Only set CAML_LD_LIBRARY_PATH for system switches opam-repository#16406 which specifies a custom line to be added to the top ofld.conf. This new flag is intended for use inopamand provides a mechanism to eliminate the setting ofCAML_LD_LIBRARY_PATHby the compiler packages in opam-repository.ld.confbeginning with an explicit reference to the current or parent directory (i.e../or../) are interpreted relative to the directory containingld.conf.OCAMLLIB(orCAMLLIB) were set in the environment, then theld.confwas loaded from the directory referred to by this environment variable, and theld.conffile located in the configured Standard Library location was ignored. The runtime now loadsld.conffrom all of these locations, processing them in the same order.I have presented all of these changes as one PR because while the individual parts of it can be separated, they all alter the same part of the runtime and don't recombine cleanly. While overhauling this infrequently-altered section of the runtime, I propose some further alterations:
ld.confis made line-ending agnostic - Unix and Cygwin builds now consistently ignore CR characters at the end of each line1stublibsdirectory, nor add it told.confld.confis exposed as a primitive which is now used byocamlc, instead of having both C and OCaml implementations of the "same" algorithm. My reason for proposing this is twofold: firstly, the two implementations even before this PR are inconsistent (as the special cases in the testsuite reveal) and, secondly, with the other changes proposed here, the two implementations become considerably larger, and thus the benefit of sharing the code is bigger. The commit unifying the two implementations is intentionally towards the end of the series (cf. the deletions inbytecomp/dll.mlin the penultimate commit, versus the deletions inbytecomp/dll.mlfor the overall PR)As far as I know, the only thing apart from OCaml which parses
ld.confisocamlfind, and the patch for that isn't particularly involved (see, for example, dra27/ocamlfind#7bd58cb). The compiler drivers use the prefix+to refer to the Standard Library (e.g. in-I +unix). Co-opting this notation forld.confdoesn't work in combination with the change to loadld.conffrom all possible locations, since+refers to the effective Standard Library location. Another possibility is to refer to the prefix by a given name, for example, having the default being$/lib/ocaml/stublibsand$/lib/ocaml. Establishing the idea of$may be useful for other tools; rooting the names of files in the prefix rather than the directory containing the file.Footnotes
The nefarious user who absolutely must have directories in
ld.confwhich end with CR characters is able to encode this by including a trailing slash after the name... ↩