Add a profile option to select the codegen backend #9118

bjorn3 · 2021-01-31T15:48:00Z

This makes it possible to compile only specific crates with a certain codegen backend.

I have tested this by compiling Bevy with cg_llvm, but one of the examples using cg_clif.

By the way I noticed that many unstable profile options are not checked when using profile overrides.

rust-highfive · 2021-01-31T15:48:03Z

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

bjorn3 · 2021-01-31T15:50:24Z

cc @khyperia This may be useful for rust-gpu.

bjorn3 · 2021-01-31T15:56:00Z

I think it would be nice to have a .cargo/config.toml option too, but I don't think there is a way to specify it on a per-crate basis there.

bors · 2021-02-03T23:34:36Z

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

alexcrichton · 2021-02-09T22:26:57Z

We discussed this during the Cargo meeting today, and we were somewhat wary to land this. We have tried to keep hardcoded paths out of manifests historically, and we would ideally prefer to avoid continuing to do so. We were also not certain about landing a feature whose future in rustc itself is uncertain, for example we're not certain how the long-term design of this feature will play out in rustc and whether it'll continue to map well to a profile option. For these reasons we're hesitant to land this PR at this time.

One option we discussed though which could be a possibility would be to add a rustflags option to profiles. This would be in addition to flags specified in other locations and would be a way to specify flags in the profile to select the codegen backend. In a way it's a sort of escape hatch that could be used for testing rustc changes with Cargo.

alexcrichton · 2021-02-09T22:28:32Z

Ah another concern that we had about this is that profiles are target-independent, but what you'd probably want is a target-specific profile for something like the codegen backend. For example the current cranelift backend doesn't support the same array of targets LLVM supports, so configuring and using this can be difficult for projects that do cross compilation. This is something we'd like to solve in the future (e.g. target-specific profile options), but is too large of a feature to implement inline here.

bjorn3 · 2021-02-09T22:47:52Z

We have tried to keep hardcoded paths out of manifests historically, and we would ideally prefer to avoid continuing to do so.

This is one of the reasons I would also like to be able to specify it in .cargo/config.toml which doesn't have to be checked in. I do have a rustc PR though that will distribute cg_clif as rustup component, thus making the hardcoded path here unnecessary.

bjorn3 · 2021-02-13T16:07:00Z

I think it would be nice to have a .cargo/config.toml option too, but I don't think there is a way to specify it on a per-crate basis there.

Looks like that is already supported: https://doc.rust-lang.org/cargo/reference/config.html#profilenamepackagename

bjorn3 · 2021-02-13T16:07:56Z

src/doc/src/reference/unstable.md

+serde = "1.0.117"
+
+[profile.dev.package.foo]
+codegen-backend = "/path/to/librustc_codegen_cranelift.so"


This could become

Suggested change

codegen-backend = "/path/to/librustc_codegen_cranelift.so"

codegen-backend = "cranelift"

once rust-lang/rust#81746 lands.

bors · 2021-02-23T22:16:37Z

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

joshtriplett · 2021-08-03T20:06:36Z

I think @alexcrichton's response still stands: we don't want to use full paths to codegen backends. It might make sense to have just a name of a codegen backend, pass that to rustc, and then rustc can find and load the codegen backend in its normal way. Or, it's possible that this should be an aspect of the target. But either way, the PR as submitted isn't something we want to merge.

I think the next step on this is a proposal, not a PR. That proposal should be based on codegen backend names, not full paths. And ideally, that proposal should start in rustc, and cargo should just expose that rustc option; cargo shouldn't be in the business of finding rustc's backends for it, rustc should know how to load cg_clif or cg_gcc or cg_spirv.

bjorn3 · 2021-08-03T20:13:00Z

This PR already implements passing the name as option. It literally passes the value through to rustc as -Zcodegen-backend, which supports both absolute paths as well as backend names for backends in the sysroot. As cg_clif isn't distributed through rustup yet, it is currently necessary to do /path/to/librustc_codegen_cranelift.so, but once it is available, "cranelift" will automatically start to work when the right rustup component is installed. I can update the doc example.

joshtriplett · 2021-08-03T20:35:06Z

@bjorn3 Ah, I see!

If you could change the Cargo support to require that the option be an identifier, rather than a path, then I'd be happy to see this go in in that form.

bjorn3 · 2021-08-03T20:48:44Z

Using a path could be useful for testing purposes. In any case I don't think using a path will ever be stabilized on the rustc side as it isn't possible to compile a codegen backend for stable anyway outside of the rust build system. If you want I can make it a warning or forbid it in cargo though.

joshtriplett · 2021-08-03T20:58:28Z

Using a path could be useful for testing purposes.

For testing purpose, I think people can use RUSTFLAGS.

If you want I can make it a warning or forbid it in cargo though.

Please make it a hard error to pass anything other than an identifier, and then I'd be happy to fcp-merge this.

This makes it possible to compile only specific crates with a certain codegen backend.

bjorn3 · 2021-08-04T16:25:47Z

For testing purpose, I think people can use RUSTFLAGS.

That doesn't allow changing the codegen backend just for certain crates.

Please make it a hard error to pass anything other than an identifier, and then I'd be happy to fcp-merge this.

Where should I add this code?

joshtriplett · 2021-08-04T20:33:29Z

@bjorn3 wrote:

That doesn't allow changing the codegen backend just for certain crates.

Acknowledged, but I still don't think we should have a Cargo config option that takes an exact path to a codegen backend. (Personally, I'd love to have a Cargo config option for specifying per-crate RUSTFLAGS.)

Where should I add this code?

In TomlProfile::validate, I think.

bjorn3 · 2021-08-05T13:26:14Z

In TomlProfile::validate, I think.

Done

src/cargo/util/toml/mod.rs

joshtriplett · 2021-08-05T15:10:11Z

I posted one fix for the validation logic. Apart from that, there's a failing test on iOS; I think the profile test you updated needs to keep the "split_debuginfo": "{...}" to match platforms where that has a different default.

Co-authored-by: Josh Triplett <[email protected]>

bjorn3 · 2021-08-05T15:19:16Z

I think the profile test you updated needs to keep the "split_debuginfo": "{...}" to match platforms where that has a different default.

I think so too. I think it would be nice to enable the feature of serde_json that sorts hashmaps to keep them deterministic. That would significantly reduce the test output diff.

joshtriplett · 2021-08-05T16:16:53Z

src/cargo/util/toml/mod.rs

+
+ if let Some(codegen_backend) = &self.codegen_backend {
+ features.require(Feature::codegen_backend())?;
+ if codegen_backend.contains(|c| !c.is_ascii_alphanumeric() && c != '_') {


Suggested change

if codegen_backend.contains(|c| !c.is_ascii_alphanumeric() && c != '_') {

if codegen_backend.contains(|c: char| !c.is_ascii_alphanumeric() && c != '_') {

Sorry, that CI failure was my fault with the previous code suggestion.

I actually only noticed the bail!() change. Rustc uses the existence of . to distinguish between builtin and external backends.

I didn't know that. We can always expand what this allows if rustc adds a built-in backend that needs a broader set of potential inputs.

bjorn3 · 2021-08-06T18:55:04Z

CI passed.

joshtriplett · 2021-08-06T20:43:16Z

@bors r

bors · 2021-08-06T20:43:17Z

📌 Commit e96309a has been approved by joshtriplett

bors · 2021-08-06T20:43:23Z

⌛ Testing commit e96309a with merge 33edacd...

bors · 2021-08-06T21:07:01Z

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 33edacd to master...

Update cargo 9 commits in cc17afbb0067b1f57d8882640f63b2168d5b7624..b51439fd8b505d4800a257acfecf3c69f81e35cf 2021-08-02 20:28:08 0000 to 2021-08-09 18:40:05 0000 - Deduplicate entries in cargo --list (rust-lang/cargo#9773) - Include aliases with other commands (rust-lang/cargo#9764) - Add a profile option to select the codegen backend (rust-lang/cargo#9118) - remove useless conversions (rust-lang/cargo#9617) - collapse nested if blocks (rust-lang/cargo#9613) - Refactor fake_file() away from cargo_command tests (rust-lang/cargo#9767) - Update cargo-platform to 0.1.2 (rust-lang/cargo#9762) - Bump to the latest jobserver dependency (rust-lang/cargo#9760) - Fix semver check for rust 1.54.0 (rust-lang/cargo#9763)

rust-highfive assigned ehuss Jan 31, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 31, 2021

bjorn3 force-pushed the profile_codegen_backend_option branch from 12d44e3 to c5b64cd Compare January 31, 2021 18:34

bjorn3 force-pushed the profile_codegen_backend_option branch 3 times, most recently from 799ebe8 to 2c9b26e Compare February 6, 2021 18:06

bjorn3 mentioned this pull request Feb 6, 2021

Add config-based path shorthands #9144

Closed

bjorn3 force-pushed the profile_codegen_backend_option branch 3 times, most recently from bd9959c to 0de6c28 Compare February 13, 2021 15:25

bjorn3 commented Feb 13, 2021

View reviewed changes

joshtriplett closed this Aug 3, 2021

joshtriplett reopened this Aug 3, 2021

bjorn3 added 2 commits August 4, 2021 18:20

Add a profile option to select the codegen backend

538d59a

This makes it possible to compile only specific crates with a certain codegen backend.

Update test

88da239

bjorn3 force-pushed the profile_codegen_backend_option branch from 0de6c28 to 88da239 Compare August 4, 2021 16:26

Only allow internal backends

2d73a9d

joshtriplett reviewed Aug 5, 2021

View reviewed changes

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved

bjorn3 and others added 2 commits August 5, 2021 17:15

Update error message

cd642e1

Co-authored-by: Josh Triplett <[email protected]>

Fix test

1aa8fd5

joshtriplett reviewed Aug 5, 2021

View reviewed changes

Fix compile error

e96309a

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 Aug 6, 2021

bors merged commit 33edacd into rust-lang:master Aug 6, 2021

bors mentioned this pull request Aug 6, 2021

Ability to specify the output name for a bin target different from the crate name #9627

Merged

bjorn3 deleted the profile_codegen_backend_option branch August 8, 2021 10:27

ehuss mentioned this pull request Aug 10, 2021

Update cargo rust-lang/rust#87928

Merged

bjorn3 mentioned this pull request Aug 29, 2021

Can't build on Windows 10 with x86_64-pc-windows-gnu; GHA not actually cross-compiling either? rust-lang/rustc_codegen_cranelift#1193

Closed

ehuss added this to the 1.56.0 milestone Feb 6, 2022

weihanglo mentioned this pull request Jun 11, 2023

codegen-backend doesn't allow arbitrary pathnames #12249

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a profile option to select the codegen backend #9118

Add a profile option to select the codegen backend #9118

bjorn3 commented Jan 31, 2021

rust-highfive commented Jan 31, 2021

bjorn3 commented Jan 31, 2021

bjorn3 commented Jan 31, 2021

bors commented Feb 3, 2021

alexcrichton commented Feb 9, 2021

alexcrichton commented Feb 9, 2021

bjorn3 commented Feb 9, 2021

bjorn3 commented Feb 13, 2021

bjorn3 Feb 13, 2021

bors commented Feb 23, 2021

joshtriplett commented Aug 3, 2021

bjorn3 commented Aug 3, 2021 •

edited

Loading

joshtriplett commented Aug 3, 2021

bjorn3 commented Aug 3, 2021

joshtriplett commented Aug 3, 2021

bjorn3 commented Aug 4, 2021

joshtriplett commented Aug 4, 2021

bjorn3 commented Aug 5, 2021

joshtriplett commented Aug 5, 2021

bjorn3 commented Aug 5, 2021

joshtriplett Aug 5, 2021

bjorn3 Aug 5, 2021

joshtriplett Aug 6, 2021

bjorn3 commented Aug 6, 2021

joshtriplett commented Aug 6, 2021

bors commented Aug 6, 2021

bors commented Aug 6, 2021

bors commented Aug 6, 2021

	codegen-backend = "/path/to/librustc_codegen_cranelift.so"
	codegen-backend = "cranelift"

	if codegen_backend.contains(\|c\| !c.is_ascii_alphanumeric() && c != '_') {
	if codegen_backend.contains(\|c: char\| !c.is_ascii_alphanumeric() && c != '_') {

Add a profile option to select the codegen backend #9118

Add a profile option to select the codegen backend #9118

Conversation

bjorn3 commented Jan 31, 2021

rust-highfive commented Jan 31, 2021

bjorn3 commented Jan 31, 2021

bjorn3 commented Jan 31, 2021

bors commented Feb 3, 2021

alexcrichton commented Feb 9, 2021

alexcrichton commented Feb 9, 2021

bjorn3 commented Feb 9, 2021

bjorn3 commented Feb 13, 2021

bjorn3 Feb 13, 2021

Choose a reason for hiding this comment

bors commented Feb 23, 2021

joshtriplett commented Aug 3, 2021

bjorn3 commented Aug 3, 2021 • edited Loading

joshtriplett commented Aug 3, 2021

bjorn3 commented Aug 3, 2021

joshtriplett commented Aug 3, 2021

bjorn3 commented Aug 4, 2021

joshtriplett commented Aug 4, 2021

bjorn3 commented Aug 5, 2021

joshtriplett commented Aug 5, 2021

bjorn3 commented Aug 5, 2021

joshtriplett Aug 5, 2021

Choose a reason for hiding this comment

bjorn3 Aug 5, 2021

Choose a reason for hiding this comment

joshtriplett Aug 6, 2021

Choose a reason for hiding this comment

bjorn3 commented Aug 6, 2021

joshtriplett commented Aug 6, 2021

bors commented Aug 6, 2021

bors commented Aug 6, 2021

bors commented Aug 6, 2021

bjorn3 commented Aug 3, 2021 •

edited

Loading