Remove missing_tools config #107830

tharunsuresh-code · 2023-02-09T05:13:48Z

Request to check if run_cargo function has been appropriately modified and what changes need to be made

rustbot · 2023-02-09T05:13:56Z

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

Please see the contribution instructions for more information.

onur-ozkan · 2023-02-09T08:37:53Z

Seems like changes made in this PR can't be passed in the pipelines currently. Please make sure all the pipelines gets passed successfully then we can review the changes.

src/bootstrap/tool.rs

onur-ozkan

Could you please also squash your commits into single one?

src/bootstrap/tool.rs

tharunsuresh-code · 2023-02-11T11:24:57Z

Could you please also squash your commits into single one?

Done!

onur-ozkan

This looks great! Just one more note before taking this into master.

src/bootstrap/tool.rs

tharunsuresh-code · 2023-02-12T04:05:54Z

This looks great! Just one more note before taking this into master.

Sure, the code looks better!

onur-ozkan

thanks for the review fixes!

onur-ozkan · 2023-02-12T08:46:42Z

@bors r+ rollup

bors · 2023-02-12T08:46:44Z

📌 Commit e679bd0 has been approved by ozkanonur

It is now in the queue for this repository.

jyn514 · 2023-02-09T05:26:12Z

src/bootstrap/dist.rs

-            .ensure(tool::RustAnalyzer { compiler, target })
-            .expect("rust-analyzer always builds");
+            .ensure(tool::RustAnalyzer { compiler, target });
+            // .expect("rust-analyzer always builds");


Suggested change

// .expect("rust-analyzer always builds");

jyn514 · 2023-02-12T15:49:28Z

src/bootstrap/tool.rs

+            source_type: SourceType::Submodule,
+            extra_features: Vec::new(),
+            allow_features: "",
+        });

        let build_cred = |name, path| {
            // These credential helpers are currently experimental.


This comment is now outdated.

jyn514 · 2023-02-12T15:50:25Z

src/bootstrap/tool.rs

-        if is_expected && !duplicates.is_empty() {
-            eprintln!(
-                "duplicate artifacts found when compiling a tool, this \
-                      typically means that something was recompiled because \
-                      a transitive dependency has different features activated \
-                      than in a previous build:\n"
-            );
-            let (same, different): (Vec<_>, Vec<_>) =
-                duplicates.into_iter().partition(|(_, cur, prev)| cur.2 == prev.2);
-            if !same.is_empty() {
-                eprintln!(
-                    "the following dependencies are duplicated although they \
-                      have the same features enabled:"


This check was removed and not replaced with anything. That seems ok to me, we don't check this for the compiler itself, but wanted to note that explicitly so it's easy to find in the future.

Why is this being removed? The duplicate artifact check is important for ensuring that some dependencies are not unnecessarily built multiple times which can increase CI time. There's a large amount of infrastructure (particularly the workspace-hack) that this would unravel.

Also, these changes seem unrelated to removing missing-tools. Is there a full description of what this PR is doing and why?

Remove missing_tools config Fixes rust-lang#79249 Request to check if run_cargo function has been appropriately modified and what changes need to be made

matthiaskrgr · 2023-02-12T21:27:54Z

Something broke miri in the rollup ^ and I can't see any changes to miri itself, wondering if it was this one...
@bors rollup=iffy

bors · 2023-02-13T00:22:02Z

⌛ Testing commit e679bd0 with merge efc262b6b3ce38809dde9031e3ef9289ab6eea55...

tharunsuresh-code · 2023-02-14T07:10:42Z

So do I need to check this in a windows environment? Because my Linux compilation in local works just fine

onur-ozkan · 2023-02-15T00:26:30Z

So do I need to check this in a windows environment?

You can catch this error which is raised on linux environment.

Because my Linux compilation in local works just fine.

I guess you run the x86_64 compilations because I can reproduce the error.

Here is a little help for reproducing the error:

config.toml that is used in failed pipeline

changelog-seen = 2

[llvm]
static-libstdcpp = true
download-ci-llvm = false

[build]
extended = true
docs = false
print-step-timings = true
metrics = true
submodules = false
locked-deps = true
cargo-native-static = true
configure-args = ['--enable-extended', '--disable-docs']

[rust]
codegen-units-std = 1
remap-debuginfo = true
verbose-tests = true
dist-src = false
channel = "nightly"
debuginfo-level-std = 1

[dist]
compression-formats = ["xz"]

command that is used in failed pipeline

python3 x.py dist --host mips64el-unknown-linux-gnuabi64 --target mips64el-unknown-linux-gnuabi64

I tried this in ubuntu 22.04 container(because I can't get some of the required packages on my host system for this particular test) and I can confirm the exact same error raises.

p.s: Most likely you can cut some of the parts from config.toml and reduce the compilation time.

tharunsuresh-code · 2023-02-15T07:04:18Z

Thanks for the detailed steps! I will try it out

tharunsuresh-code · 2023-02-17T12:57:17Z

From what I gather, it looks an issue with libffi-sys. It does not have any "mips" architecture defined in their arch.rs file here -> https://github.com/tov/libffi-rs/blob/master/libffi-sys-rs/src/arch.rs

Should I take it up with the maintainers of that repository asking to include mips and mips64 architecture?

Specific error:

error[E0425]: cannot find value `FFI_TRAMPOLINE_SIZE` in this scope
   --> libffi-sys-rs/src/lib.rs:165:25
    |
165 |     pub tramp: [c_char; FFI_TRAMPOLINE_SIZE],
    |                         ^^^^^^^^^^^^^^^^^^^ not found in this scope
    |
note: these constants exist but are inaccessible
   --> libffi-sys-rs/src/arch.rs:34:9

Note - The i686 windows build error is different and occurs in llvm build, which I am not able to debug yet

jyn514 · 2023-02-19T15:36:02Z

@tharunsuresh-code where do you see that error? when building which tool? I don't see it in the github actions logs.

jyn514 · 2023-02-19T15:36:39Z

The LLVM error looks spurious so I think we can retry this PR: #108227

@bors retry

jyn514 · 2023-02-19T15:37:43Z

oh no I'm wrong, I missed the miri error @ozkanonur pointed out. Sorry for the noise.

@bors r-

jyn514 · 2023-02-19T15:42:25Z

Should I take it up with the maintainers of that repository asking to include mips and mips64 architecture?

Yes, that seems like the right thing to do :) mips64-unknown-linux-gnuabi64 is "Tier 2 with Host Tools" so theoretically it should have all these tools available.

cc @RalfJung @Mark-Simulacrum in case you have opinions on whether we should require Miri to be available on tier 2 targets; right now it's allowed to be missing.

tharunsuresh-code · 2023-02-19T16:02:52Z

The LLVM error looks spurious so I think we can retry this PR: #108227

@bors retry

Thanks for pointing this out! I was trying to recreate the windows error and was having successful builds on my local windows machine 😅

Should I take it up with the maintainers of that repository asking to include mips and mips64 architecture?

Yes, that seems like the right thing to do :) mips64-unknown-linux-gnuabi64 is "Tier 2 with Host Tools" so theoretically it should have all these tools available.

cc @RalfJung @Mark-Simulacrum in case you have opinions on whether we should require Miri to be available on tier 2 targets; right now it's allowed to be missing.

Thanks, @jyn514! Created an issue -> tov/libffi-rs#67

RalfJung · 2023-02-19T21:24:13Z

in case you have opinions on whether we should require Miri to be available on tier 2 targets; right now it's allowed to be missing.

No strong opinion. Assuming that it doesn't put much of a burden on Miri itself (which I doubt it does; adding support for more targets is tricky but I don't quite see how more host triples could be a problem), I'd leave this to the people that work on these targets. Though it seems you did run into libffi issues... we could totally disable FFI support on hosts not supported by libffi.

However using Miri on those targets might require passing something like --target x86_64-unknown-linux-gnu (which works on any host) to ensure maximal Miri support. That said, the Miri test suite actually checks --target mips64-unknown-linux-gnuabi64 to ensure things work on a big-endian target, so that particular triple should work fine.

bors · 2023-02-20T19:58:03Z

☔ The latest upstream changes (presumably #108268) made this pull request unmergeable. Please resolve the merge conflicts.

jyn514 · 2023-02-20T22:24:20Z

we could totally disable FFI support on hosts not supported by libffi

Hmm, how could we do that without hard coding the target name? I worry that we'll disable FFI for the target, it will get supported upstream, and then it'll stay unsupported in miri indefinitely because we don't notice.

RalfJung · 2023-02-21T07:12:53Z

Yeah we'd rely on someone reporting an issue to unlock this in Miri.

commit 1c808f18903f43ca6898a1452caf2481627b3d4e Merge: e679bd0 3200982 Author: Tharun Suresh <stharunvikram@gmail.com> Date: Tue Feb 21 21:42:46 2023 +0530 Merge branch 'master' of https://github.com/rust-lang/rust commit e679bd0 Author: Tharun Suresh <stharunvikram@gmail.com> Date: Sun Feb 12 09:37:41 2023 +0530 Remove missing_tools config rust-lang#107830

ehuss · 2023-02-24T01:48:09Z

src/bootstrap/tool.rs

@@ -696,7 +548,6 @@ impl Step for Cargo {
                tool: name,
                mode: Mode::ToolRustc,
                path,
-                is_optional_tool: true,


This is intentionally set to be optional. These credential programs had some risk that they wouldn't compile in all environments. I suppose if you can get CI to pass on all targets, then it should be ok. But I want to point out that this is a risky change, and I'm a little nervous about doing this.

ehuss · 2023-02-24T01:49:11Z

src/bootstrap/config.rs

@@ -202,7 +202,6 @@ pub struct Config {
    pub save_toolstates: Option<PathBuf>,
    pub print_step_timings: bool,
    pub print_step_rusage: bool,
-    pub missing_tools: bool,


Removing this will break anyone who has it set in their config. Is there a procedure for removing settings in such a way to alert people ahead-of-time that their config will no longer work?

ehuss · 2023-02-24T01:50:52Z

src/bootstrap/tool.rs

-        if is_expected && !duplicates.is_empty() {
-            eprintln!(
-                "duplicate artifacts found when compiling a tool, this \
-                      typically means that something was recompiled because \
-                      a transitive dependency has different features activated \
-                      than in a previous build:\n"
-            );
-            let (same, different): (Vec<_>, Vec<_>) =
-                duplicates.into_iter().partition(|(_, cur, prev)| cur.2 == prev.2);
-            if !same.is_empty() {
-                eprintln!(
-                    "the following dependencies are duplicated although they \
-                      have the same features enabled:"


Why is this being removed? The duplicate artifact check is important for ensuring that some dependencies are not unnecessarily built multiple times which can increase CI time. There's a large amount of infrastructure (particularly the workspace-hack) that this would unravel.

Also, these changes seem unrelated to removing missing-tools. Is there a full description of what this PR is doing and why?

bors · 2023-03-08T01:26:42Z

☔ The latest upstream changes (presumably #108877) made this pull request unmergeable. Please resolve the merge conflicts.

onur-ozkan · 2023-04-28T08:19:04Z

I will close this due to inactivity. Feel free to re-open it when you decide to work on this.

jyn514 · 2023-12-07T14:17:26Z

tov/libffi-rs#67 was fixed, might be worth rechecking if miri builds on mips now

RalfJung · 2023-12-07T15:21:00Z

MIPS is a tier 3 target now (#115238), so if this PR was blocked on "Miri should build on all tier 2 targets" then MIPS is not even a concern any more.

rustbot assigned onur-ozkan Feb 9, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Feb 9, 2023

compiler-errors changed the title ~~Remove missing_tools config #79249~~ Remove missing_tools config Feb 9, 2023

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Feb 9, 2023

onur-ozkan 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 Feb 9, 2023

tharunsuresh-code commented Feb 9, 2023

View reviewed changes

src/bootstrap/tool.rs Outdated Show resolved Hide resolved

onur-ozkan reviewed Feb 10, 2023

View reviewed changes

src/bootstrap/tool.rs Outdated Show resolved Hide resolved

onur-ozkan reviewed Feb 11, 2023

View reviewed changes

src/bootstrap/tool.rs Show resolved Hide resolved

src/bootstrap/tool.rs Outdated Show resolved Hide resolved

tharunsuresh-code force-pushed the master branch from d3b0e3a to 1d3aac4 Compare February 11, 2023 11:23

onur-ozkan requested changes Feb 11, 2023

View reviewed changes

src/bootstrap/tool.rs Outdated Show resolved Hide resolved

tharunsuresh-code added a commit to tharunsuresh-code/rust that referenced this pull request Feb 12, 2023

Remove missing_tools config rust-lang#107830

e679bd0

tharunsuresh-code force-pushed the master branch from 95029aa to e679bd0 Compare February 12, 2023 04:08

onur-ozkan approved these changes Feb 12, 2023

View reviewed changes

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 12, 2023

jyn514 reviewed Feb 12, 2023

View reviewed changes

matthiaskrgr mentioned this pull request Feb 12, 2023

Rollup of 7 pull requests #107963

Closed

erikdesjardins mentioned this pull request Feb 19, 2023

CI: spurious error building LLVM on mingw: ranlib.exe: could not create temporary file whilst writing archive: no more archived files #108227

Open

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 19, 2023

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 19, 2023

tharunsuresh-code force-pushed the master branch from 1684426 to 68431a6 Compare February 22, 2023 07:52

ehuss reviewed Feb 24, 2023

View reviewed changes

onur-ozkan closed this Apr 28, 2023

RalfJung mentioned this pull request Dec 7, 2023

Remove missing_tools config #79249

Closed

onur-ozkan mentioned this pull request Dec 28, 2023

Remove usage of deprecated missing-tools bootstrap flag #119373

Merged

Remove missing_tools config #107830

Remove missing_tools config #107830

Conversation

tharunsuresh-code commented Feb 9, 2023 • edited by jyn514 Loading

rustbot commented Feb 9, 2023

onur-ozkan commented Feb 9, 2023 • edited Loading

onur-ozkan left a comment

Choose a reason for hiding this comment

tharunsuresh-code commented Feb 11, 2023

onur-ozkan left a comment

Choose a reason for hiding this comment

tharunsuresh-code commented Feb 12, 2023

onur-ozkan left a comment • edited Loading

Choose a reason for hiding this comment

onur-ozkan commented Feb 12, 2023

bors commented Feb 12, 2023

jyn514 Feb 9, 2023

Choose a reason for hiding this comment

jyn514 Feb 12, 2023

Choose a reason for hiding this comment

jyn514 Feb 12, 2023

Choose a reason for hiding this comment

ehuss Feb 24, 2023

Choose a reason for hiding this comment

matthiaskrgr commented Feb 12, 2023

bors commented Feb 13, 2023

tharunsuresh-code commented Feb 14, 2023

onur-ozkan commented Feb 15, 2023 • edited Loading

tharunsuresh-code commented Feb 15, 2023

tharunsuresh-code commented Feb 17, 2023 • edited Loading

jyn514 commented Feb 19, 2023

jyn514 commented Feb 19, 2023

jyn514 commented Feb 19, 2023

jyn514 commented Feb 19, 2023

tharunsuresh-code commented Feb 19, 2023 • edited Loading

RalfJung commented Feb 19, 2023

bors commented Feb 20, 2023

jyn514 commented Feb 20, 2023

RalfJung commented Feb 21, 2023 via email

ehuss Feb 24, 2023

Choose a reason for hiding this comment

ehuss Feb 24, 2023

Choose a reason for hiding this comment

ehuss Feb 24, 2023

Choose a reason for hiding this comment

bors commented Mar 8, 2023

onur-ozkan commented Apr 28, 2023

jyn514 commented Dec 7, 2023

RalfJung commented Dec 7, 2023 • edited Loading

tharunsuresh-code commented Feb 9, 2023 •

edited by jyn514

Loading

onur-ozkan commented Feb 9, 2023 •

edited

Loading

onur-ozkan left a comment •

edited

Loading

onur-ozkan commented Feb 15, 2023 •

edited

Loading

tharunsuresh-code commented Feb 17, 2023 •

edited

Loading

tharunsuresh-code commented Feb 19, 2023 •

edited

Loading

RalfJung commented Dec 7, 2023 •

edited

Loading