Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions src/workspace/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,14 @@ use std::fmt::{self, Write};
use std::path::PathBuf;
use std::str::FromStr;

use anyhow::{Error, bail};
use cargo_util::ProcessBuilder;
use serde::{Deserialize, Serialize};
use tracing::debug;

use crate::GlobalContext;
use crate::resolver::ResolveBehavior;
use crate::util::errors::CargoResult;
use crate::util::indented_lines;
use anyhow::{Error, bail};
Comment thread
Kobzol marked this conversation as resolved.
Resolved
use cargo_util::ProcessBuilder;
use serde::{Deserialize, Serialize};
use tracing::debug;

pub const SEE_CHANNELS: &str = "See https://doc.rust-lang.org/book/appendix-07-nightly-rust.html for more information \
about Rust release channels.";
Expand Down Expand Up @@ -409,7 +408,7 @@ enum Status {
}

/// Config for the `-Zembed-metadata` option.
#[derive(Debug, Default, Deserialize)]
#[derive(Debug, Default)]
pub enum EmbedMetadata {
/// Embed metadata in .rlib files, the original rustc behavior.
Embed,
Expand Down Expand Up @@ -883,6 +882,7 @@ macro_rules! unstable_cli_options {
unstable_cli_options!(
// Permanently unstable features:
allow_features: Option<AllowFeatures> = ("Allow *only* the listed unstable features"),
#[serde(deserialize_with = "deserialize_embed_metadata")]
embed_metadata: EmbedMetadata = ("Avoid embedding metadata in library artifacts"),
print_im_a_teapot: bool,

Expand Down Expand Up @@ -1051,6 +1051,18 @@ where
Ok(Some(v))
}

fn deserialize_embed_metadata<'de, D>(deserializer: D) -> Result<EmbedMetadata, D::Error>

@weihanglo weihanglo Jul 27, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have one simple test verifying it works?

View changes since the review

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably in tests/testsuite/config.rs

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that, the problem is that the config tests use .get, and that expects the returned type to implement Deserialize (which this type now doesn't) 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😞
Probably we could just use serde(rename) oh those two yes|no variants and serde(skip) unset?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is different than this PR's bool config though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to have third option, "yes"|"no" string value may be better than bool

@Kobzol Kobzol Jul 27, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The third option is really just equivalent to None (i.e. the user didn't set any value).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The what about this?

     pub fn should_embed_metadata(&self) -> bool {
-        match self.cli_unstable().embed_metadata {
-            EmbedMetadata::Embed => true,
-            EmbedMetadata::DoNotEmbed => false,
-            EmbedMetadata::Unset => true,
-        }
+        self.cli_unstable().embed_metadata.unwrap_or(true)
     }
// ...
 unstable_cli_options!(
     // Permanently unstable features:
     allow_features: Option<AllowFeatures> = ("Allow *only* the listed unstable features"),
-    embed_metadata: EmbedMetadata = ("Avoid embedding metadata in library artifacts"),
+    embed_metadata: Option<bool> = ("Avoid embedding metadata in library artifacts"),
     print_im_a_teapot: bool,

It should work under this requirement right?

The third variant is needed for enabling the flag by default on nightly, to recognize if the flag was set by the user or not.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, Option<bool> also works, though I found the enum nicer, because I could document the individual variants :) But I'm also fine with Option<bool>.

where
D: serde::Deserializer<'de>,
{
let data = Option::<bool>::deserialize(deserializer)?;
match data {
Some(true) => Ok(EmbedMetadata::Embed),
Some(false) => Ok(EmbedMetadata::DoNotEmbed),
None => Ok(EmbedMetadata::Unset),
}
}

#[derive(Debug, Copy, Clone, Default, Deserialize, Ord, PartialOrd, Eq, PartialEq)]
#[serde(default)]
pub struct GitFeatures {
Expand Down