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

Canceling initial check build does not work #13348

Closed
RalfJung opened this issue Oct 5, 2022 · 9 comments · Fixed by #13552
Closed

Canceling initial check build does not work #13348

RalfJung opened this issue Oct 5, 2022 · 9 comments · Fixed by #13552
Labels
C-bug Category: bug

Comments

@RalfJung
Copy link
Member

RalfJung commented Oct 5, 2022

To reproduce:

  • start vscode with RA on a rustc checkout that is out-of-date (e.g., touch library/core/src/lib.rs)
  • once it gets going, do ./x.py test src/test/ui in a terminal
  • now it waits for the lock that RA grabbed for its check-build. in RA, do "cancel running flychecks".

Expected behavior:

  • the build in the terminal starts

Actual behavior:

  • the check build keeps going. I can see in htop that there's still a check build running, started by RA

Strangely, that build is out-of-sync with the flycheck status bar at the bottom of the vscode window: usually it shows there the last crate whose check was completed, but in cases like this that status bar either shows some very early crate from the library (despite the check build already being long past that, and working on rustc crates), or it doesn't show any completed crates at all. It seems almost as if RA would kick off two checkbuilds, a flycheck and something else, and even the flycheck is blocked waiting for that other build to complete. I have proc macro support disabled though so I cannot think of why it would do that.

rust-analyzer version: rust-analyzer version: 0.3.1221-standalone

rustc version: (rustc bootstrap)

relevant settings:
workspace settings

    "rust-analyzer.rustc.source": "./Cargo.toml",
    "rust-analyzer.linkedProjects": [
        "./Cargo.toml",
        //"./src/bootstrap/Cargo.toml"
    ],
    "rust-analyzer.checkOnSave.overrideCommand": [
        "./x.py",
        "check",
        "--json-output",
        "library/std",
        "compiler/rustc",
        //"compiler/rustc_codegen_cranelift",
        //"src/tools/miri",
    ],
    // This also affects proc macros.
    "rust-analyzer.cargo.buildScripts.overrideCommand": [
        "./x.py",
        "check",
        "--json-output",
        "compiler/rustc",
    ],
    "rust-analyzer.rustfmt.overrideCommand": [
        "./build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt",
        "--edition=2021"
    ],
    "files.watcherExclude": {
        "*rustc*/src/llvm-project/**": true,
        "*rustc*/build/**": true,
    },
    "files.exclude": {
        "src/llvm-project/**": true,
        "build/**": true
    },
    "rust-analyzer.cargo.buildScripts.enable": false,
    "rust-analyzer.procMacro.enable": false,

user settings

    "rust-analyzer.lens.enable": false,
    "rust-analyzer.cargo.buildScripts.useRustcWrapper": false,
    "rust-analyzer.cargo.buildScripts.enable": false,
    "rust-analyzer.procMacro.enable": false,
    //
    "rust-analyzer.diagnostics.disabled": [
        "unlinked-file", // I guess for "normal" projects unlinked files are strange, for me they are common
        "unresolved-module", // https://github.com/rust-lang/rust-analyzer/issues/9173
        "unresolved-extern-crate", // https://github.com/rust-lang/rust-analyzer/issues/12926
        "type-mismatch", // https://github.com/rust-lang/rust-analyzer/issues/1109
    ],
@bjorn3
Copy link
Member

bjorn3 commented Oct 5, 2022

The initial cargo check is not a flycheck. It is a special invocation with a rustc wrapper that makes it only run build scripts and compile proc macros (you have this option to use the wrapper disabled. likely because it is incompatible with x.py) The result of this is necessary for usage by rust-analyzer itself to better analyse your code. It doesn't run again when you save, so aborting it would effectively break those features until you restart rust-analyzer or edit Cargo.toml or build.rs to retrigger it.

@Veykril
Copy link
Member

Veykril commented Oct 5, 2022

Buildscripts and proc-macros are disabled though so the wrapper shouldn't run at all. We kick off one check invocation per workspace loaded, in your case the two linked projects.So two flychecks make sense here, but cancelling should cancel both of them :/ Ah wait one of them is commented out nvm.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 5, 2022

@bjorn3 yeah that's why I disabled proc-macros and build scripts -- they make the startup time on a 'cold' rustc checkout just too big.

I can also see in htop that the invocation is for library/std compiler/rustc, i.e. it doesn't match the buildScripts.overrideCommand.

@Veykril
Copy link
Member

Veykril commented Oct 5, 2022

Is this maybe x.py being weird again? That is, what happens to the x.py command if r-a kills the invocation of it, does x.py kill the subsequent check invocations it started?

@RalfJung
Copy link
Member Author

RalfJung commented Oct 5, 2022

Ctrl-C on x.py in the terminal works as expected and kills cargo immediately.

@Veykril Veykril added the C-bug Category: bug label Oct 5, 2022
@RalfJung
Copy link
Member Author

RalfJung commented Oct 6, 2022

Yeah maybe it's a problem in the interaction with x.py. When I save a file while x.py check is running, it also first completes the old build and then re-starts a new check build. I assume it is supposed to cancel the running build first?

@bjorn3
Copy link
Member

bjorn3 commented Oct 6, 2022

Ctrl-C on x.py in the terminal works as expected and kills cargo immediately.

Ctrl-C sends SIGINT to the entire process group. Rust-analyzer doesn't set up a process group I believe and as such it only kills bootstrap. I don't believe bootstrap has code to kill it's inner cargo instance. See https://github.com/rust-lang/cargo/blob/c39193a7c5ecb65508257d0b1bdde579570ee5f2/src/cargo/util/job.rs for some explanation (on windows cargo has to manually set up the equivalent of a process group to get the right behavior)

@RalfJung
Copy link
Member Author

RalfJung commented Oct 6, 2022

So we either need x.py to tear down inner processes, or RA to send a signal to the entire process tree?

@bjorn3
Copy link
Member

bjorn3 commented Oct 6, 2022

I think rust-analyzer should set up a process group for each subprocess it spawns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants