-
Notifications
You must be signed in to change notification settings - Fork 30
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
Relocatable ocamlfind #72
Relocatable ocamlfind #72
Conversation
I did some playing around with this branch while investigating ocaml/dune#8931. I'm trying to build https://github.com/ocaml/Zarith directly (without dune) after installing ocamlfind from this branch without passing the new flag to
That's suspicious to me because I thought that My experimental setup is to clone this branch and run:
...and then to run I haven't tried out the new configure option yet but just wanted to raise this as it looks like a regression. The above experiment works for me when I repeat it with the tip of the master branch of ocamlfind. |
@gridbugs That's a good point, I missed the final |
Trying this out on macos and it looks like it's trying to read the (non-existent)
(this was while trying to build https://github.com/gridbugs/oneshot-webserver-app/) |
Indeed, I probably need to copy more of how the relocatable compiler deals with determining the executable name. Looks like on NextStep I can use Maybe that's actually better, then I can replace |
c93a9a0
to
061afee
Compare
Since your latest changes I can now use this on macos and linux to build packages with dune's package management features. One issue I run into now is that using the
The problem goes away if you pass |
I tried using this to build the In summary:
More details in the linked issue. |
I've removed the C parts, since I don't think there's a way to determine the location from OCaml on platforms other than Linux without at least some C code (e.g. both macOS and Windows). Instead I made it look up The |
src/findlib/Makefile
Outdated
@@ -179,6 +180,9 @@ depend: *.ml *.mli fl_meta.ml fl_metascanner.ml findlib_config.ml topfind.ml | |||
.mli.cmi: | |||
$(OCAMLC) $(OPAQUE) $(OCAMLC_FLAGS) $(OCAMLFIND_OCAMLFLAGS) -c $< | |||
|
|||
.c.o: | |||
$(OCAMLC) $(OPAQUE) $(OCAMLC_FLAGS) $(OCAMLFIND_OCAMLFLAGS) -c $< | |||
|
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 guess this can go away after rmoving the C files again?
let len = String.length prefix in | ||
match String.starts_with ~prefix path with | ||
| false -> None | ||
| true -> Some (String.sub path len (String.length path - len)) |
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.
any special handling of /
needed? E.g.
prefix="$PREFIX"
andpath="$PREFIXgoeson"
: returnsgoeson
prefix="$PREFIX"
andpath="$PREFIX/d3/foo"
: returns/d3/foo
(absolute!)
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.
Not in this function because it is should indeed return that absolute path (I've renamed it since), so returning the absolute part should not be a problem, given it is later only used to Filename.concat
with the computed prefix.
However with that concern in mind, if the path was $PREFIX/d3/foo
and we couldn't compute a Findlib.location
it would wrongly add /d3/foo
to the search path. I've changed it to (silently) discard the path in such case. The alternative would be to expand $PREFIX/d3/footo a relative
d3/foo` but that behavior seems a bit surprising and unexpected.
ok, this seems to be the best PR so far to add some relocatibility. As far as I understand, the PR needs a special version of OCaml to work properly, right? Can we test for this in the configure script, and only allow relocatable paths when this version is available? Regarding the |
For the relocation it shouldn't need any specific compiler, the compiler patches are for relocating the compiler itself. I'm developing this on a normal OCaml 3.08.4 switch. Originally I thought I could use the same approach as the relocatable compiler patches do, but thanks to @gridbugs testing it turned out that I had to scrap the idea and the environment variables set by OPAM and Dune are probably a better option. Excellent hint wrt. I thought about unifying |
83afb46
to
c4be238
Compare
Hi @gerdstolpmann, I think this is ready as far as we are concerned. @gridbugs and I tested it on macOS and Linux as well as in Dune. I've just force-pushed to skip over all the intermediate steps in finding a proper solution and merging the commits fixing issues that were discovered on the way. I took some extra care that Could you take a look again and tell me if there's anything missing? |
I think it is good now. There is a conflict now, however, can you fix? |
This unifies both templates to use only one single topfind template so every change happens in both "older" and "newer" topfind files
c4be238
to
a31715e
Compare
@gerdstolpmann I've rebased upon the current |
Relying on opam and dune environment variables seems a bit hacky and brittle. And it limits the usefulness to one package manager and one build system. (Sorry for the churn!) Isn't the real relocation problem the absolute paths inside findlib.conf? And the solution for that is making sure that configuration files can be portable through dune-like path variables? For example, I have a Windows findlib.conf: destdir="Y:\\source\\scoutapps\\#s\\site-lib"
path="C:\\Users\\beckf\\AppData\\Local\\Programs\\DkCoder\\coder\\h\\Env\\lib;C:\\Users\\beckf\\AppData\\Local\\Programs\\DkCoder\\coder\\h\\Env\\share\\DkSDKCoder_Us\\DkDev\\src;C:\\Users\\beckf\\AppData\\Local\\Programs\\DkCoder\\coder\\h\\Env\\share\\DkSDKCoder_Us\\DkFs\\src;C:\\Users\\beckf\\AppData\\Local\\Programs\\DkCoder\\coder\\h\\Env\\share\\DkSDKCoder_Us\\DkNet\\src;C:\\Users\\beckf\\AppData\\Local\\Programs\\DkCoder\\coder\\h\\Env\\share\\DkSDKCoder_Us\\DkStdRestApis\\src;Y:\\source\\scoutapps\\#s\\site-lib;C:\\Users\\beckf\\AppData\\Local\\DkCoder\\site\\96697830-7d79-49f5-8d79-2066101f0ef9"
stdlib="C:\\Users\\beckf\\AppData\\Local\\Programs\\DkCoder\\coder\\h\\Env\\lib\\ocaml"
ldconf="Y:\\source\\scoutapps\\#s\\ld.conf" In the findlib example it can be made partially portable with a destdir="%{prefix}%{dirsep}#s%{dirsep}site-lib"
path="C:\\Users\\beckf\\AppData\\Local\\Programs\\DkCoder\\coder\\h\\Env\\lib%{pathsep}C:\\Users\\beckf\\AppData\\Local\\Programs\\DkCoder\\coder\\h\\Env\\share\\DkSDKCoder_Us\\DkDev\\src%{pathsep}C:\\Users\\beckf\\AppData\\Local\\Programs\\DkCoder\\coder\\h\\Env\\share\\DkSDKCoder_Us\\DkFs\\src%{pathsep}C:\\Users\\beckf\\AppData\\Local\\Programs\\DkCoder\\coder\\h\\Env\\share\\DkSDKCoder_Us\\DkNet\\src%{pathsep}C:\\Users\\beckf\\AppData\\Local\\Programs\\DkCoder\\coder\\h\\Env\\share\\DkSDKCoder_Us\\DkStdRestApis\\src%{pathsep}%{prefix}%{dirsep}#s%{dirsep}site-lib%{pathsep}C:\\Users\\beckf\\AppData\\Local\\DkCoder\\site\\96697830-7d79-49f5-8d79-2066101f0ef9"
stdlib="C:\\Users\\beckf\\AppData\\Local\\Programs\\DkCoder\\coder\\h\\Env\\lib\\ocaml"
ldconf="%{prefix}%{dirsep}#s%{dirsep}ld.conf" Then it would be the responsibility of |
@jonahbeckford At some point findlib needs to know the prefix, sooner or later. The first version of the PR tried to infer the prefix from the path of the running executable, but for the documented reasons we can't do this. Hence the environment variables. You can move the point in time when the prefix is needed, but you can't avoid it. |
I'm puzzled (but it can be addressed in a subsequent release) as to why this ended up with C stubs, rather than just using |
@dra27 I don't see any C stubs in the version that was merged? |
No, I mean in the original version that got discarded for this less good version relying on specific environment variables? |
Well, there are a couple of downsides of
In particular the latter means that any executable-lookup method is not a good solid basis for figuring out the install location. But this doesn't mean such a method couldn't complement it. I've mainly merged this PR because this way it is easy to test, and I'm sure there will be some good suggestions for further improvement. |
Indeed - I'm aware the library part needs additional work (it's the same thing with compiler-libs in Relocatable OCaml). Granted, |
This PR mirrors the work on the compiler on making it relocatable. The way it works it adds a new option for configure that can be used to instead of hardcoding the absolute path, it will resolve
$PREFIX
to the installation location using various means (readlink
, reading environment variables etc).