Skip to content
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

Work around LLVM OCAML binding installation failure #47846

Merged
merged 2 commits into from
Feb 15, 2018

Conversation

roblabla
Copy link
Contributor

Hello,

I have OCaml installed on my machine, and compiling rust systematically fails when LLVM attempts installing the OCaml bindings in /usr/lib/ocaml, which is write-protected. Here are the logs: https://gist.github.com/roblabla/3f147914c5df627c9d97ab311ba133ad

Some digging around the issue reveals:

The problem seems to be that LLVM_OCAML_INSTALL_PATH is set to OCAML_STDLIB_PATH by default, which is in /usr/lib/ocaml, instead of the prefix.

This PR "fixes" the issue by setting LLVM_OCAML_INSTALL_PATH to usr/lib/ocaml. I haven't found a way to make LLVM not build OCaml, which would probably be a superior fix.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 29, 2018
@kennytm
Copy link
Member

kennytm commented Jan 29, 2018

Could we simply disable all bindings like libretro/Lakka#431

@kennytm kennytm added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jan 29, 2018
@roblabla
Copy link
Contributor Author

I'd love to, but Lakka seems to be using autoconf instead of cmake. I'm not sure how we're supposed to disable bindings using the cmake build system (It's probably possible, but i've never used cmake before, and searches have been unsuccessful so far).

@roblabla
Copy link
Contributor Author

More recent versions of lakka seem to patch out the bindings out of the CMakeLists https://github.com/libretro/Lakka-LibreELEC/blob/Lakka-V2.1-dev/packages/lang/llvm/patches/llvm-0001-disable-go-and-ocaml-bindings.patch

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 29, 2018
@mattico
Copy link
Contributor

mattico commented Jan 29, 2018

I assume that LLVM would accept a patch that lets you disable the bindings. I'll try to do this, wish me luck!

(Which does not mean this PR shouldn't be accepted. If I get a patch in, it will end up in LLVM 7, so this will help in the meantime)

@@ -155,6 +155,7 @@ impl Step for Llvm {
.define("WITH_POLLY", "OFF")
.define("LLVM_ENABLE_TERMINFO", "OFF")
.define("LLVM_ENABLE_LIBEDIT", "OFF")
.define("LLVM_OCAML_INSTALL_PATH", "usr/lib/ocaml")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the point of this environment is to declare an invalid binary so LLVM can't find the real ocaml, could we make it more explicit like

           .define("LLVM_OCAML_INSTALL_PATH", "/__disable_ocaml_bindings")

The current value looks like a typo 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's so it installs ocaml bindings in there instead of /usr/lib/ocaml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically ocaml will be found automagically, and the bindings will be installed in LLVM_OCAML_INSTALL_PATH

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roblabla Ah ok. Could you copy this explanation into the code?

@nikomatsakis
Copy link
Contributor

r? @kennytm -- I don't know much about this =)

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2018
@ghost
Copy link

ghost commented Feb 2, 2018

It fixed the issue on my gentoo system, thanks ;)

Though I don't think that's the correct place to do it - LLVM_OCAML_INSTALL_PATH was meant to be set by package management software e.g. portage on gentoo ( llvm-mirror/llvm@a7d748b ).

In my opinion, it would be better to add feature that disables building bindings in llvm (hmm, gentoo does -DOCAMLFIND=NO to disable building ocaml bindings in system llvm install).

@kennytm
Copy link
Member

kennytm commented Feb 2, 2018

We could set it as var_os("LLVM_OCAML_INSTALL_PATH").unwrap_or_else(|| OsStr::new("usr/lib/ocaml")) to respect the existing value.

@mattico
Copy link
Contributor

mattico commented Feb 2, 2018

https://reviews.llvm.org/D42026 beat me to it, but I have an alternate patch ready if this one is not accepted.

@shepmaster
Copy link
Member

Ping from triage, @roblabla ! Will you have time to address the most recent feedback?

@roblabla
Copy link
Contributor Author

Welp, thanks for the heads up, this had flown under my radar. I'll fix that.

@roblabla
Copy link
Contributor Author

Alright sorry for the wait. I added a comment explaining the problem, and included kennytm's suggestion of using LLVM_OCAML_INSTALL_PATH if it is provided.

@kennytm
Copy link
Member

kennytm commented Feb 11, 2018

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 11, 2018

📌 Commit 3c01dea has been approved by kennytm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 11, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Feb 12, 2018
Work around LLVM OCAML binding installation failure

Hello,

I have OCaml installed on my machine, and compiling rust systematically fails when LLVM attempts installing the OCaml bindings in `/usr/lib/ocaml`, which is write-protected. Here are the logs: https://gist.github.com/roblabla/3f147914c5df627c9d97ab311ba133ad

Some digging around the issue reveals:

- The code that finds if OCaml is installed, and sets the bindings to be compiled/installed: https://github.com/llvm-mirror/llvm/blob/b24a45d2e9f4fc10c3f9e16172104910b38637f2/cmake/config-ix.cmake#L612
- https://github.com/llvm-mirror/llvm/blob/b24a45d2e9f4fc10c3f9e16172104910b38637f2/bindings/ocaml/llvm/CMakeLists.txt Some code that does the installation.

The problem seems to be that `LLVM_OCAML_INSTALL_PATH` is set to `OCAML_STDLIB_PATH` by default, which is in `/usr/lib/ocaml`, instead of the prefix.

This PR "fixes" the issue by setting `LLVM_OCAML_INSTALL_PATH` to `usr/lib/ocaml`. I haven't found a way to make LLVM not build OCaml, which would probably be a superior fix.
bors added a commit that referenced this pull request Feb 12, 2018
Rollup of 8 pull requests

- Successful merges: #47846, #48033, #48087, #48114, #48126, #48130, #48133, #48151
- Failed merges:
kennytm added a commit to kennytm/rust that referenced this pull request Feb 13, 2018
Work around LLVM OCAML binding installation failure

Hello,

I have OCaml installed on my machine, and compiling rust systematically fails when LLVM attempts installing the OCaml bindings in `/usr/lib/ocaml`, which is write-protected. Here are the logs: https://gist.github.com/roblabla/3f147914c5df627c9d97ab311ba133ad

Some digging around the issue reveals:

- The code that finds if OCaml is installed, and sets the bindings to be compiled/installed: https://github.com/llvm-mirror/llvm/blob/b24a45d2e9f4fc10c3f9e16172104910b38637f2/cmake/config-ix.cmake#L612
- https://github.com/llvm-mirror/llvm/blob/b24a45d2e9f4fc10c3f9e16172104910b38637f2/bindings/ocaml/llvm/CMakeLists.txt Some code that does the installation.

The problem seems to be that `LLVM_OCAML_INSTALL_PATH` is set to `OCAML_STDLIB_PATH` by default, which is in `/usr/lib/ocaml`, instead of the prefix.

This PR "fixes" the issue by setting `LLVM_OCAML_INSTALL_PATH` to `usr/lib/ocaml`. I haven't found a way to make LLVM not build OCaml, which would probably be a superior fix.
bors added a commit that referenced this pull request Feb 13, 2018
Rollup of 14 pull requests

- Successful merges: #47784, #47846, #48033, #48083, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48163, #48165, #48167
- Failed merges:
kennytm added a commit to kennytm/rust that referenced this pull request Feb 14, 2018
Work around LLVM OCAML binding installation failure

Hello,

I have OCaml installed on my machine, and compiling rust systematically fails when LLVM attempts installing the OCaml bindings in `/usr/lib/ocaml`, which is write-protected. Here are the logs: https://gist.github.com/roblabla/3f147914c5df627c9d97ab311ba133ad

Some digging around the issue reveals:

- The code that finds if OCaml is installed, and sets the bindings to be compiled/installed: https://github.com/llvm-mirror/llvm/blob/b24a45d2e9f4fc10c3f9e16172104910b38637f2/cmake/config-ix.cmake#L612
- https://github.com/llvm-mirror/llvm/blob/b24a45d2e9f4fc10c3f9e16172104910b38637f2/bindings/ocaml/llvm/CMakeLists.txt Some code that does the installation.

The problem seems to be that `LLVM_OCAML_INSTALL_PATH` is set to `OCAML_STDLIB_PATH` by default, which is in `/usr/lib/ocaml`, instead of the prefix.

This PR "fixes" the issue by setting `LLVM_OCAML_INSTALL_PATH` to `usr/lib/ocaml`. I haven't found a way to make LLVM not build OCaml, which would probably be a superior fix.
bors added a commit that referenced this pull request Feb 14, 2018
bors added a commit that referenced this pull request Feb 15, 2018
bors added a commit that referenced this pull request Feb 15, 2018
@kennytm kennytm merged commit 3c01dea into rust-lang:master Feb 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants