feat(metadata): Add profiles information to `cargo metadata` #12449

calavera · 2023-08-05T15:59:09Z

I do understand that the team is not accepting PRs for issues that have not been accepted. However, I believe these changes are scoped well enough to be considered for review. Nevertheless, feel free to close this PR without reviewing, I understand that I didn't follow the contribution rules. In honesty, I wrote the code before I saw that warning, and I didn't want to keep the code for myself 😅

What does this PR try to resolve?

This change adds profile information to the output of cargo metadata. The output only changes if the user has included any specific profile settings in their Cargo.toml file, it doesn't expose default profiles that have not been modified. I opted for serializing this information only when there is information present because I believe it's a minimal incremental improvement that's easier to test, review and accept than if I tried to expose profile information that is not explicitly in Cargo.toml files.

How should we test and review this PR?

I've added integration tests in the metadata test suite for these changes.

rustbot · 2023-08-05T15:59:13Z

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

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

calavera · 2023-08-05T17:15:47Z

Tests are failing because of something that looks like unrelated to these changes. I ran those same tests locally, and they pass:

 error: the feature `lang_items` is internal to the compiler or standard library
 --> src/lib.rs:4:12
  |
4 | #![feature(lang_items)]
  |            ^^^^^^^^^^
  |

ehuss · 2023-08-05T19:12:30Z

Sorry about the hiccup, I have posted #12450 to fix the CI issue.

calavera · 2023-08-06T01:32:16Z

Thanks for the quick fix @ehuss. I've rebased from master and tests pass now as expected.

bors · 2023-08-25T16:17:48Z

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

ehuss · 2023-08-30T17:52:34Z

src/cargo/ops/cargo_output_metadata.rs

+    let workspace_profiles = match ws.root_maybe() {
+        MaybePackage::Virtual(vm) => vm.profiles().cloned(),
+        _ => None, // regular packages include the profile information under their package section
+    };


I'm a little concerned about this approach. For example, if you have a non-virtual workspace, this could make it quite difficult to determine what the profiles are for the workspace, since you would need to somehow fish it out of the packages array. I would suggest just including ws.profiles() as-is.

sounds good, I can make that update.

ehuss · 2023-08-30T18:01:56Z

@rfcbot fcp merge

@rust-lang/cargo Just checking if people would be ok with adding the TOML profiles from the manifest in cargo metadata. I believe this will also show up with cargo read-manifest.

rfcbot · 2023-08-30T18:12:40Z

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

default-config (feat(metadata): Add profiles information to cargo metadata #12449 (comment))
"JSON message compatibility" (feat(metadata): Add profiles information to cargo metadata #12449 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

epage · 2023-08-30T21:59:59Z

src/cargo/core/package.rs

+    #[serde(skip_serializing_if = "Option::is_none")]
+    profiles: Option<TomlProfiles>,


Cargo only looks at the profile settings in the Cargo.toml manifest at the root of the workspace. Profile settings defined in dependencies will be ignored.

imo we should only serialize profiles at the workspace level so people get something more akin to expected cargo behavior

that would simplify things and aligns with @ehuss comment above. I can remove it from there and just call it profiles in the output metadata.

epage · 2023-08-30T22:03:00Z

src/cargo/ops/cargo_output_metadata.rs

+    #[serde(skip_serializing_if = "Option::is_none")]
+    workspace_profiles: Option<TomlProfiles>,


The output only changes if the user has included any specific profile settings in their Cargo.toml file, it doesn't expose default profiles that have not been modified.

For me, the question is what the intended use case is we are covering. All callers will need to reinvent cargo's profile system to fully interpret this, including reading from config. Is that ok? Can we change this in the future if we find it doesn't work?

In my case, Cargo's default profiles are not relevant. I want to know profile information that people set in their projects explicitly. I'll be happy to expose the default profile data if you think it'd be useful, but I don't know where to find it.

One specific use case doesn't tell us much though. We are looking at adding a new feature and we need to consider other users and future users to make sure we are providing the right thing.

Do we understand enough to move forward either direction?

In my usecase I would like to have the default profiles available. My usecase would be to provide CMake users an option to select one of the available cargo profiles. For this I would like to have
a) name of the profile
b) path of the profile (mainly because dev maps to debug)

This list should include the default profiles, although I'm not sure about test and bench though, since those profiles are weird and artifacts don't land at a predictable location due to the hash being added.

epage · 2023-08-30T22:04:01Z

src/cargo/ops/cargo_output_metadata.rs

+    #[serde(skip_serializing_if = "Option::is_none")]
+    workspace_profiles: Option<TomlProfiles>,


Seems like this should be profiles instead of workspace_profiles

It looks like we only use the workspace prefix when qualifying things workspace root, workspace members) but otherwise we don't (e.g. metadata, resolve)

epage · 2023-08-30T22:05:21Z

@rfcbot concern default-config

See #12449 (comment)

calavera · 2023-09-01T04:06:33Z

@ehuss @epage thanks for the feedback. I've made some changes based on your comments.

@epage I looked into exposing the default profiles too. It looks like I could do that if I change the implementation to use Profiles::new, and then expose the original profiles with a new function like this one:

+
+    /// Returns the original profiles written by the user in the manifest and config.
+    pub fn original_profiles(&self) -> &BTreeMap<InternedString, TomlProfile> {
+        &self.original_profiles
+    }

I would like to check with you that that's the right direction because I will have to update all the metadata output tests, and I don't want you all to review something that's not what you expect. Let me know what you think.

epage · 2023-09-01T15:29:46Z

I would like to check with you that that's the right direction

My question isn't so much about what needs to be changed but exploring the requirements for the change itself. We need to understand that before we know what the right change is.

calavera · 2023-09-01T16:05:18Z

@epage that makes sense. My usecase is described in this comment: #11228 (comment)

This change adds profile information to the output of `cargo metadata`. I opted for serializing this information only when there is information present to minimize changes in the output. Signed-off-by: David Calavera <david.calavera@gmail.com>

Signed-off-by: David Calavera <david.calavera@gmail.com>

Implement a new method Profiles::all that returns all profiles in the workspace, including the root and predefined profiles. This new method follows Cargo resolution rules to resolve profile inheritance and ordering. Signed-off-by: David Calavera <david.calavera@gmail.com>

Signed-off-by: David Calavera <david.calavera@gmail.com>

calavera · 2023-09-25T03:40:05Z

@epage, I've been looking at the use cases in this comment, and #11228. I think you're right, and exposing only information defined by the user in the Cargo.toml file is not enough. I did some digging today, and I created a new function that extracts all profiles in a workspace, including root and predefined profiles. The metadata command serializes that list instead of toml definitions. It's a bigger change than I wanted to make, but it's mostly decoupling functions from the Profiles struct.

Let me know what you think.

weihanglo · 2023-09-26T09:26:36Z

tests/testsuite/script.rs

@@ -1081,7 +1081,84 @@ fn cmd_metadata_with_embedded() {
        "target_directory": "[ROOT]/home/.cargo/target/[..]",
        "version": 1,
        "workspace_root": "[..]/foo",
-        "metadata": null
+        "metadata": null,
+        "profiles": {


Should we reuse DEFAULT_PROFILES here?

weihanglo · 2023-09-26T10:00:24Z

src/cargo/core/profiles.rs

+fn root_profiles(
+    profiles: &BTreeMap<InternedString, TomlProfile>,
+) -> Vec<(InternedString, ProfileMaker)> {
+    vec![


We dont need a Vec here I guess? an [(InternedString, ProfileMaker); 2] should be sufficient.

weihanglo · 2023-09-26T10:01:34Z

src/cargo/core/profiles.rs

+/// Returns the built-in profiles (not including dev/release, which are
+/// "root" profiles).
+fn predefined_profiles() -> Vec<(&'static str, TomlProfile)> {
+    vec![


ditto. [(&'static str, TomlProfile); 3] is good to go.

weihanglo · 2023-09-26T10:15:14Z

tests/testsuite/metadata.rs

@@ -6,6 +6,84 @@ use cargo_test_support::registry::Package;
 use cargo_test_support::{basic_bin_manifest, basic_lib_manifest, main_file, project, rustc_host};
 use serde_json::json;

+pub(crate) const DEFAULT_PROFILES: &str = r#"{
+  "bench": {
+    "codegen_backend": null,


Should codegen_backend be skipped, as the feature is not stable yet?

weihanglo · 2023-09-26T10:15:40Z

tests/testsuite/metadata.rs

+    "debug_assertions": false,
+    "debuginfo": 0,
+    "incremental": false,
+    "lto": "false",


it seems odd that lto false is a stringified "false".

weihanglo · 2023-09-26T10:40:30Z

src/cargo/core/profiles.rs

+        match self {
+            Strip::None => "none".serialize(s),
+            Strip::Named(n) => n.serialize(s),
+        }


Suggested change

match self {

Strip::None => "none".serialize(s),

Strip::Named(n) => n.serialize(s),

}

s.collect_str(self)

BTW it would be great if we can make commit atomic. Like, this change can fit it one commit of its own.

weihanglo · 2023-09-26T10:51:15Z

src/cargo/core/profiles.rs

+
+        for (name, profile) in &definitions {
+            let name = *name;
+            if let Some(maker) = create_profile_maker(name, profile, &makers, &definitions)? {
+                makers.insert(name, maker);
+            }
+        }


Duplicate?

Suggested change

for (name, profile) in &definitions {

let name = *name;

if let Some(maker) = create_profile_maker(name, profile, &makers, &definitions)? {

makers.insert(name, maker);

}

}

weihanglo · 2023-09-26T10:56:51Z

src/cargo/core/profiles.rs

        Ok(profile_makers)
    }

-    /// Returns the hard-coded directory names for built-in profiles.
-    fn predefined_dir_names() -> HashMap<InternedString, InternedString> {


This seems like a refactor in order to reuse code between Profiles::new and Proflies::all. I would suggest commit the refactor in a commit, and the other for adding the new feature. It helps both review and tracing Git history.

weihanglo · 2023-09-26T11:05:03Z

@rfcbot concern "JSON message compatibility"

This PR insta-stabilizes all profiles settings in JSON format. However, we do have some issues around JSON message compatbility, especially when we want to add more types to a field later. The issue is tracked in #12377.

Do we want to resolve #12377 first then merge this PR? Should we look into each field and determine the serialization format one by one? Or both?

calavera · 2023-09-26T15:59:58Z

This PR insta-stabilizes all profiles settings in JSON format. However, we do have some issues around JSON message compatbility, especially when we want to add more types to a field later. The issue is tracked in #12377.

I was not aware of that ticket, but I'll take a look. I basically didn't want to change anything in the format for backwards compatibility, but if there are concerns about the format itself, I can help to stabilize it before landing all these changes.

calavera · 2023-10-05T09:31:40Z

I'm going to close this PR for now. After digging through the code, I have concerns about how best expose profiles in the metadata. The lack of stable format doesn't help either. I'll try to send a different PR with some code improvements that make the profiles module more testeable, that should give people more confidence to look into this part of the codebase.

weihanglo · 2023-10-06T03:54:31Z

Thank you @calavera for working hard on this from different angles!

rustbot assigned ehuss Aug 5, 2023

rustbot added A-manifest Area: Cargo.toml issues Command-metadata S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2023

calavera force-pushed the profile_info_in_metadata branch from 08b44bf to 17fc760 Compare August 5, 2023 16:45

calavera force-pushed the profile_info_in_metadata branch from 17fc760 to 18c4f98 Compare August 6, 2023 00:00

ehuss reviewed Aug 30, 2023

View reviewed changes

ehuss added the T-cargo Team: Cargo label Aug 30, 2023

rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Aug 30, 2023

epage reviewed Aug 30, 2023

View reviewed changes

calavera force-pushed the profile_info_in_metadata branch 3 times, most recently from 3623e27 to 30af6de Compare September 1, 2023 04:00

rustbot added the A-profiles Area: profiles label Sep 25, 2023

calavera force-pushed the profile_info_in_metadata branch 2 times, most recently from 5960ae1 to fb3a571 Compare September 25, 2023 02:15

calavera force-pushed the profile_info_in_metadata branch from fb3a571 to d480856 Compare September 25, 2023 02:33

calavera added 4 commits September 24, 2023 19:40

Remove profiles information from the package structure.

a83cca4

Signed-off-by: David Calavera <david.calavera@gmail.com>

Revert changes to TomlProfile

ffb2401

Signed-off-by: David Calavera <david.calavera@gmail.com>

calavera force-pushed the profile_info_in_metadata branch from d480856 to ffb2401 Compare September 25, 2023 02:41

Fix comment about root profiles.

b2f9e3b

Signed-off-by: David Calavera <david.calavera@gmail.com>

weihanglo reviewed Sep 26, 2023

View reviewed changes

calavera closed this Oct 5, 2023

rfcbot removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Oct 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(metadata): Add profiles information to `cargo metadata` #12449

feat(metadata): Add profiles information to `cargo metadata` #12449

calavera commented Aug 5, 2023 •

edited

Loading

rustbot commented Aug 5, 2023

calavera commented Aug 5, 2023

ehuss commented Aug 5, 2023

calavera commented Aug 6, 2023

bors commented Aug 25, 2023

ehuss Aug 30, 2023

calavera Aug 30, 2023

ehuss commented Aug 30, 2023

rfcbot commented Aug 30, 2023 •

edited

Loading

epage Aug 30, 2023

calavera Aug 30, 2023

epage Aug 30, 2023

calavera Aug 30, 2023

epage Sep 1, 2023

jschwe Sep 3, 2023

epage Aug 30, 2023

epage commented Aug 30, 2023

calavera commented Sep 1, 2023

epage commented Sep 1, 2023

calavera commented Sep 1, 2023

calavera commented Sep 25, 2023 •

edited

Loading

weihanglo Sep 26, 2023

weihanglo Sep 26, 2023

weihanglo Sep 26, 2023

weihanglo Sep 26, 2023

weihanglo Sep 26, 2023

weihanglo Sep 26, 2023

weihanglo Sep 26, 2023

weihanglo Sep 26, 2023

weihanglo commented Sep 26, 2023

calavera commented Sep 26, 2023

calavera commented Oct 5, 2023

weihanglo commented Oct 6, 2023

		#[serde(skip_serializing_if = "Option::is_none")]
		profiles: Option<TomlProfiles>,

		#[serde(skip_serializing_if = "Option::is_none")]
		workspace_profiles: Option<TomlProfiles>,

feat(metadata): Add profiles information to cargo metadata #12449

feat(metadata): Add profiles information to cargo metadata #12449

Conversation

calavera commented Aug 5, 2023 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Aug 5, 2023

calavera commented Aug 5, 2023

ehuss commented Aug 5, 2023

calavera commented Aug 6, 2023

bors commented Aug 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Aug 30, 2023

rfcbot commented Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage commented Aug 30, 2023

calavera commented Sep 1, 2023

epage commented Sep 1, 2023

calavera commented Sep 1, 2023

calavera commented Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weihanglo commented Sep 26, 2023

calavera commented Sep 26, 2023

calavera commented Oct 5, 2023

weihanglo commented Oct 6, 2023

feat(metadata): Add profiles information to `cargo metadata` #12449

feat(metadata): Add profiles information to `cargo metadata` #12449

calavera commented Aug 5, 2023 •

edited

Loading

rfcbot commented Aug 30, 2023 •

edited

Loading

calavera commented Sep 25, 2023 •

edited

Loading