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

bootstrap: remove lldb dist packaging #72058

Merged
merged 1 commit into from
May 14, 2020
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented May 9, 2020

The lldb-preview rustup package is missing on every single target, and has never been shipped beyond x86_64-apple-darwin. It was removed in #62592 which landed around a year ago, and there's not been demand that we re-enable it since, so we're now removing support entirely to cleanup the code a bit.

The hope is that this will also kill the useless "lldb-preview" row on https://rust-lang.github.io/rustup-components-history/.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(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 May 9, 2020
@Mark-Simulacrum
Copy link
Member

r? @Mark-Simulacrum

LLDB was disabled in #62592, nearly a year ago. My guess is since I haven't seen anything since then suggesting re-enabling it we can also remove support (can always resurrect the small amount of support left from git history anyway).

I think we'll want to remove the options related to lldb as well; probably the best way to do that is to remove the lldb_enabled on config and then handle rustc errors from there. We'll also want to drop the configure.py --lldb option.

@RalfJung
Copy link
Member Author

@Mark-Simulacrum okay, I did that. And then in the last commit I removed some more things that looked outdated, but I am not sure I understand enough of this so please check carefully.

Remaining occurrences of "lldb":

src/bootstrap/test.rs
1091:            cmd.arg("--lldb-python").arg("/usr/bin/python3");
1093:            cmd.arg("--lldb-python").arg(builder.python());
1109:        let lldb_exe = "lldb";
1110:        let lldb_version = Command::new(lldb_exe)
1115:        if let Some(ref vers) = lldb_version {
1116:            cmd.arg("--lldb-version").arg(vers);
1117:            let lldb_python_dir = run(Command::new(lldb_exe).arg("-P")).ok();
1118:            if let Some(ref dir) = lldb_python_dir {
1119:                cmd.arg("--lldb-python-dir").arg(dir);

src/bootstrap/dist.rs
590:        run.path("src/lldb_batchmode.py")
631:            // lldb debugger scripts
632:            builder.install(&builder.src.join("src/etc/rust-lldb"), &sysroot.join("bin"), 0o755);
634:            cp_debugger_script("lldb_rust_formatters.py");
896:            "llvm-project/lldb",
897:            "llvm-project\\lldb",

@Mark-Simulacrum
Copy link
Member

Okay, this looks good to me. Let's kick off a try build to make sure at least one dist builder works (@bors try). Presuming that passes and you squash the commits down, r=me.

@mati865
Copy link
Contributor

mati865 commented May 10, 2020

Looks like bors didn't work.

@Mark-Simulacrum
Copy link
Member

I guess I was too fancy. I'll give it its own paragraph?

@bors try

@bors
Copy link
Contributor

bors commented May 10, 2020

⌛ Trying commit d41fd4482c78298c188eab7e71d2b3f8b9b349d0 with merge dd0ed6d6598328dfc60f4bdcd00eacd20db6e3ca...

@bors
Copy link
Contributor

bors commented May 10, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: dd0ed6d6598328dfc60f4bdcd00eacd20db6e3ca (dd0ed6d6598328dfc60f4bdcd00eacd20db6e3ca)

@RalfJung
Copy link
Member Author

@Mark-Simulacrum do I have to check the logs of that try build or is a pass enough?

@Mark-Simulacrum
Copy link
Member

Pass is fine. If you can squash the commits then I think we can land it.

it's not been built since a long time ago
@RalfJung
Copy link
Member Author

Done that.

@Mark-Simulacrum
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented May 10, 2020

📌 Commit dc7524b has been approved by Mark-Simulacrum

@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 May 10, 2020
@Dylan-DPC-zz
Copy link

@bors p=1

@bors
Copy link
Contributor

bors commented May 14, 2020

⌛ Testing commit dc7524b with merge 23ffeea...

@bors
Copy link
Contributor

bors commented May 14, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 23ffeea to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 14, 2020
@bors bors merged commit 23ffeea into rust-lang:master May 14, 2020
@RalfJung RalfJung deleted the no-dist-lldb branch May 14, 2020 16:38
@eddyb
Copy link
Member

eddyb commented May 28, 2020

Uhh, I was using this in order to run (the lldb side of) debuginfo tests. Am I supposed to switch to whatever other lldb I have access to? I recall that not working last time, hence dealing with local builds.

@Mark-Simulacrum
Copy link
Member

Hm if that's not working I'm not opposed to restoring this to some extent -- I think it does make sense to drop the dist bits but we can revert the "build" bits.

@eddyb
Copy link
Member

eddyb commented May 28, 2020

Will report back with the results of trying to use NixOS-provided lldb, once I can confirm that compiletest isn't skipping the lldb side of debuginfo tests. Maybe I don't need this at all, but I've relied on it for so long that I doubted I could have it working any other way.

Well, the other reason for the local build was testing the new demangler (see #60705), but I likely can easily build a patched copy of lldb just through NixOS so that isn't as big of a concern.

@eddyb
Copy link
Member

eddyb commented May 29, 2020

Interesting, I've tried lldb 7 through 10 and only 10 actually passed all tests.
I'm not sure if the two tests that lldb 9 still failed on have changed more recently and that's why they fail, but the version requirement likely explains why I recall not getting this to work w/o a local build.

Bonus: src/test/debuginfo/issue-22656.rs passes now (maybe it's just because assertions aren't enabled, heh) so I can remove the hack that deletes it every time I do a build.

@RalfJung
Copy link
Member Author

@eddyb to be clear, you were not using anything dist-related, right? That at looked like long dead code.

@eddyb
Copy link
Member

eddyb commented May 29, 2020

Yeah I was just using the local build, and only for debuginfo tests (lldb = true also enabled lldb debuginfo tests to run even w/o lldb in $PATH, not just built a local copy).

But at least now that I know I can just use lldb 10, it's less trouble to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

8 participants