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

Have rust-lldb look for the rust-enabled lldb #53973

Merged
merged 1 commit into from
Sep 8, 2018

Conversation

tromey
Copy link
Contributor

@tromey tromey commented Sep 5, 2018

We're shipping a rust-enabled lldb, but the "lldb" executable is not
installed into the "bin" directory by rustup. See the discussion in
rust-lang/rustup#1492 for
background on this decision. There, we agreed to have rust-lldb
prefer the rust-enabled lldb if it is installed. This patch changes
rust-lldb to look in the sysroot and use the lldb found there, if any.

See issue #48168

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2018
@alexcrichton
Copy link
Member

Nice! In light of #53955 and #53813 though we may actually want to switch where lldb is located as well. Does LLDB link dynamically to the LLVM shared object? If so, it may no longer work if installed at $sysroot/bin I think?

@tromey
Copy link
Contributor Author

tromey commented Sep 6, 2018

Does LLDB link dynamically to the LLVM shared object?

lldb is just built as part of the overall llvm toolchain build, so it does whatever that does.

I think it would be reasonable if #53955 made the same change for lldb.

@tromey
Copy link
Contributor Author

tromey commented Sep 6, 2018

Actually, I wonder how rust-lldb will know which target to choose.

@tromey
Copy link
Contributor Author

tromey commented Sep 6, 2018

Ah, I see that dist.rs talks in terms of targets but the PR talked in terms of host. So that explains one thing, though I still am not sure this info is available to the script.

@cuviper
Copy link
Member

cuviper commented Sep 6, 2018

You can get the host from rustc -Vv.

@tromey
Copy link
Contributor Author

tromey commented Sep 6, 2018

Thanks @cuviper - I was looking at --print and didn't know about that one.

@tromey
Copy link
Contributor Author

tromey commented Sep 6, 2018

BTW I am waiting for nightly to catch up so I can see if it does make sense to make the change. Currently things are out of sync; and I didn't see a patch to move the LLVM .so.

@alexcrichton
Copy link
Member

Sure thing!

We're shipping a rust-enabled lldb, but the "lldb" executable is not
installed into the "bin" directory by rustup.  See the discussion in
rust-lang/rustup#1492 for
background on this decision.  There, we agreed to have rust-lldb
prefer the rust-enabled lldb if it is installed.

This patch changes dist.rs to put lldb into rustlib, following what
was done for the other LLVM tools in rust-lang#53955, and then fixes rust-lldb
to prefer that lldb, if it exists.

See issue rust-lang#48168
@tromey
Copy link
Contributor Author

tromey commented Sep 7, 2018

In addition to what the commit message says, this one changes rust-lldb to skip the version check if we've decided to use the rust-enabled lldb, as it's not necessary in that case. The previous patch could sometimes emit an invalid warning if you had an old lldb in your path and the script actually decided to use the rust lldb.

@alexcrichton
Copy link
Member

@bors: r+

Looks good to me!

@bors
Copy link
Contributor

bors commented Sep 7, 2018

📌 Commit 8aae6ca has been approved by alexcrichton

@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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 7, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Sep 7, 2018
…alexcrichton

Have rust-lldb look for the rust-enabled lldb

We're shipping a rust-enabled lldb, but the "lldb" executable is not
installed into the "bin" directory by rustup.  See the discussion in
rust-lang/rustup#1492 for
background on this decision.  There, we agreed to have rust-lldb
prefer the rust-enabled lldb if it is installed.  This patch changes
rust-lldb to look in the sysroot and use the lldb found there, if any.

See issue rust-lang#48168
kennytm added a commit to kennytm/rust that referenced this pull request Sep 8, 2018
…alexcrichton

Have rust-lldb look for the rust-enabled lldb

We're shipping a rust-enabled lldb, but the "lldb" executable is not
installed into the "bin" directory by rustup.  See the discussion in
rust-lang/rustup#1492 for
background on this decision.  There, we agreed to have rust-lldb
prefer the rust-enabled lldb if it is installed.  This patch changes
rust-lldb to look in the sysroot and use the lldb found there, if any.

See issue rust-lang#48168
bors added a commit that referenced this pull request Sep 8, 2018
Rollup of 11 pull requests

Successful merges:

 - #51366 (stabilize #[panic_handler])
 - #53162 (rustdoc: collect trait impls as an early pass)
 - #53932 ([NLL] Remove base_place)
 - #53942 (Rewrite `precompute_borrows_out_of_scope` for fewer hash table lookups.)
 - #53973 (Have rust-lldb look for the rust-enabled lldb)
 - #53981 (Implement initializer() for FileDesc)
 - #53987 (rustbuild: allow configuring llvm version suffix)
 - #53993 (rustc_resolve: don't record uniform_paths canaries as reexports.)
 - #54007 (crates that provide a `panic_handler` are exempt from the `unused_extern_crates` lint)
 - #54040 (update books for next release)
 - #54050 (Update `petgraph` dependency to 0.4.13 to fix build with nightly)

Failed merges:

r? @ghost
kennytm added a commit to kennytm/rust that referenced this pull request Sep 8, 2018
…alexcrichton

Have rust-lldb look for the rust-enabled lldb

We're shipping a rust-enabled lldb, but the "lldb" executable is not
installed into the "bin" directory by rustup.  See the discussion in
rust-lang/rustup#1492 for
background on this decision.  There, we agreed to have rust-lldb
prefer the rust-enabled lldb if it is installed.  This patch changes
rust-lldb to look in the sysroot and use the lldb found there, if any.

See issue rust-lang#48168
bors added a commit that referenced this pull request Sep 8, 2018
Rollup of 10 pull requests

Successful merges:

 - #53315 (use `NonZeroU32` in `newtype_index!`macro, change syntax)
 - #53932 ([NLL] Remove base_place)
 - #53942 (Rewrite `precompute_borrows_out_of_scope` for fewer hash table lookups.)
 - #53973 (Have rust-lldb look for the rust-enabled lldb)
 - #53981 (Implement initializer() for FileDesc)
 - #53987 (rustbuild: allow configuring llvm version suffix)
 - #53993 (rustc_resolve: don't record uniform_paths canaries as reexports.)
 - #54007 (crates that provide a `panic_handler` are exempt from the `unused_extern_crates` lint)
 - #54040 (update books for next release)
 - #54050 (Update `petgraph` dependency to 0.4.13 to fix build with nightly)
@bors bors merged commit 8aae6ca into rust-lang:master Sep 8, 2018
@tromey tromey deleted the prefer-rust-enabled-lldb branch September 8, 2018 18:50
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants