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

JSON-based workspaces don't detect or read target data layout? #14673

Open
whentojump opened this issue Apr 28, 2023 · 10 comments
Open

JSON-based workspaces don't detect or read target data layout? #14673

whentojump opened this issue Apr 28, 2023 · 10 comments
Labels
C-support Category: support questions

Comments

@whentojump
Copy link
Contributor

Question

Looks like JSON-based workspace doesn't have an equivalent of target_layout filed as in Cargo-based ones?

Cargo {
cargo: CargoWorkspace,
build_scripts: WorkspaceBuildScripts,
sysroot: Result<Sysroot, Option<String>>,
rustc: Result<(CargoWorkspace, WorkspaceBuildScripts), Option<String>>,
/// Holds cfg flags for the current target. We get those by running
/// `rustc --print cfg`.
///
/// FIXME: make this a per-crate map, as, eg, build.rs might have a
/// different target.
rustc_cfg: Vec<CfgFlag>,
cfg_overrides: CfgOverrides,
toolchain: Option<Version>,
target_layout: Result<String, String>,
},
/// Project workspace was manually specified using a `rust-project.json` file.
Json {
project: ProjectJson,
sysroot: Result<Sysroot, Option<String>>,
/// Holds cfg flags for the current target. We get those by running
/// `rustc --print cfg`.
rustc_cfg: Vec<CfgFlag>,
toolchain: Option<Version>,
},

So when creating the crate graph here, we pass in an Err:

Err("rust-project.json projects have no target layout set".into()),

May I know whether this is intentional, or perhaps at least a FIXME should be tagged at this line?


Observed effects

#8813 won't work in JSON-based workspaces, e.g.

let _x = [true; 100];
  //^^ [bool; _] 

the length part is still Unknown thus displayed as an underscore.


Possible solutions? (if we are to fix it)

  • reuse this target_data_layout::get() function for JSON-based workspaces?

    if let Some(cargo_toml) = cargo_toml {
    let mut cmd = Command::new(toolchain::rustc());
    cmd.envs(extra_env);
    cmd.current_dir(cargo_toml.parent())
    .args(["-Z", "unstable-options", "--print", "target-spec-json"])
    .env("RUSTC_BOOTSTRAP", "1");
    if let Some(target) = target {
    cmd.args(["--target", target]);
    }
    match utf8_stdout(cmd) {
    Ok(it) => return Ok(it),
    Err(e) => tracing::debug!("{e:?}: falling back to querying rustc for cfgs"),
    }
    }
    // using unstable cargo features failed, fall back to using plain rustc
    let mut cmd = Command::new(toolchain::rustc());
    cmd.envs(extra_env)
    .args(["-Z", "unstable-options", "--print", "target-spec-json"])
    .env("RUSTC_BOOTSTRAP", "1");
    if let Some(target) = target {
    cmd.args(["--target", target]);
    }
    utf8_stdout(cmd)

    By the way, in this function there are two attempts of issuing rustc command. It looks to me the only difference between them is to cd <project root> or not. Idk, will the existence of Cargo.toml in the current directory affect the result of rustc +nightly -Z unstable-options --print target-spec-json?

  • derive the data layout information from this provided triple?

    pub(crate) target: Option<String>,

Thanks for your effort!

@whentojump whentojump added the C-support Category: support questions label Apr 28, 2023
@HKalbasi
Copy link
Member

We definitely want target data layout for rust-project.json based projects, and your possible solution sections looks like it is to the point. It would be great if you come with a PR.

Idk, will the existence of Cargo.toml in the current directory affect the result of rustc +nightly -Z unstable-options --print target-spec-json?

I think you can provide the custom target with --target for it?

@whentojump
Copy link
Contributor Author

whentojump commented Apr 28, 2023

Thanks for your reply!

come with a PR.

Sure. Let me try.

Idk, will the existence of Cargo.toml in the current directory affect the result of rustc +nightly -Z unstable-options --print target-spec-json?

I think you can provide the custom target with --target for it?

Yeah I agree. It is also reflected in the snippet (the cmd.args(["--target", target]) part).

But my point was, the snippet seemingly wants to extract certain information from Cargo.toml with this line:

cmd.current_dir(cargo_toml.parent())

and if this first run of rustc fails, it launches a second run of rustc without .current_dir() configuration, as a fallback.

-- I am just confused about this design, and "why this function is currently only used for Cargo-based workspaces". Anyway, let me try it for JSON-based workspaces too.

@HKalbasi
Copy link
Member

But my point was, the snippet seemingly wants to extract certain information from Cargo.toml with this line:

I guess in case of cargo, the caller of this function won't provide the target (unless it is explicitly provided as a config) and it finds the target by running rustc in the directory of the crate. So for the rust-project.json you can just provide the config.

@Veykril
Copy link
Member

Veykril commented Apr 28, 2023

By the way, in this function there are two attempts of issuing rustc command. It looks to me the only difference between them is to cd or not. Idk, will the existence of Cargo.toml in the current directory affect the result of rustc +nightly -Z unstable-options --print target-spec-json?

That's a bug, the first one should be invoking cargo with rustc as its subcommand (first argument to the command). Basically we want to run `cargo rustc -Z unstable-options --print target-spec-json" first to honor cargo configs, if that fails we fall back to using the plain rustc command.

Though for rust-project.json I'm not sure if we wanna attempt using cargo at all? Usually rust-project.json indicates a different workflow than standard cargo.

@davidbarsky
Copy link
Contributor

Though for rust-project.json I'm not sure if we wanna attempt using cargo at all? Usually rust-project.json indicates a different workflow than standard cargo.

At least in my employer's case, it's possible to invoke rustc directly (technically cargo as well, since we store the full sysroot in the repo, but I think rustc is more correct).

@Veykril
Copy link
Member

Veykril commented Apr 28, 2023

Yes, rustc is fine to do. The question is whether attempting to invoke cargo first makes sense, which I would argue it does not. AS usual, for non cargo based projects we should probably offer a way to set this in a different way/discover it differently.

@whentojump
Copy link
Contributor Author

the first one should be invoking cargo with rustc as its subcommand

I guess so according to the context :)

Then one follow-up question is: what extra infomation can cargo provide regarding the target, compared to the plain rustc? My search only leads to this unstable config parameter.

@Veykril
Copy link
Member

Veykril commented Apr 28, 2023

Running the command through cargo allows honoring target settings set through .cargo/config.toml. Running plain rustc will ignore that.

@whentojump
Copy link
Contributor Author

Ahh I see. Thanks!

@davidbarsky
Copy link
Contributor

Yes, rustc is fine to do. The question is whether attempting to invoke cargo first makes sense, which I would argue it does not. AS usual, for non cargo based projects we should probably offer a way to set this in a different way/discover it differently.

Sorry about that! I misread what you were saying.

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

No branches or pull requests

4 participants