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

Extend config to use metadata keys, like build script directives #13211

Open
epage opened this issue Dec 27, 2023 · 2 comments
Open

Extend config to use metadata keys, like build script directives #13211

epage opened this issue Dec 27, 2023 · 2 comments
Labels
A-configuration Area: cargo config files and env vars A-links Area: `links` native library links setting C-enhancement Category: enhancement S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@epage
Copy link
Contributor

epage commented Dec 27, 2023

In #11461, we were concerned about stepping on the toes of link metadata when adding new build script directives. #12201 solved this by locking down the old build script directive syntax and making the new syntax only accept metadata explicitly with the metadata key.

In doing this, we overlooked target.<triple>.<links>, see

_ => {
let val = value.string(key)?.0;
output.metadata.push((key.clone(), val.to_string()));
}
.

@epage epage added A-configuration Area: cargo config files and env vars C-enhancement Category: enhancement A-links Area: `links` native library links setting S-needs-team-input Status: Needs input from team on whether/how to proceed. labels Dec 27, 2023
@epage
Copy link
Contributor Author

epage commented Dec 27, 2023

I think, being config, we can be more flexible on compatibility and continue to move forward with #12201.

I would propose

  • We add a metadata table for consistency and so people have a place to put keys without any risk of breaking compatibility
    • We first have a transition period with an "unused key" warning if metadata is used
  • We refactor parse_links_overrides so we're less likely to break it when adding new fields to TargetCfgConfig.
  • We refactor parse_links_overrides / build script code so we can more easily ensure we update one when updating the other

I'm being hand-wavy on the refactors, unsure what would help.

@weihanglo
Copy link
Member

The proposal of metadata is exactly what is in my mind! Thanks for writing this out.

The refactor could be focused on removing manual deserialization and instead using serde. Not sure how hard it is.

@ehuss ehuss added I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting and removed I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting labels Dec 27, 2023
@weihanglo weihanglo added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. and removed S-needs-team-input Status: Needs input from team on whether/how to proceed. labels Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-links Area: `links` native library links setting C-enhancement Category: enhancement S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
Archived in project
Development

No branches or pull requests

3 participants