feat: Allow declaring cfg groups in rust-project.json, to help sharing common cfgs #17857
Conversation
rustbot
added
the
S-waiting-on-review
|
CI fails due to rustfmt screams (I wonder if it makes sense to split that one out as a separate CI check) r? @davidbarsky @bors delegate=@davidbarsky |
|
✌️ @davidbarsky, you can now approve this pull request! If @Veykril told you to " |
ChayimFriedman2
force-pushed
the
rust-project-cfg-group
branch
from
b1ffc5b to
a6a55c9
Compare
There was a problem hiding this comment.
Not opposed to this change in principle; just had two comments:
- Considering using
FxHashSet. - I don't know if the memory usage for rust-analyzer would be better (as mentioned as a motivating example in Specifying a set of
cfgs for given crates (cfggroups) #17815 by @ojeda), but I can absolutely imagine that the file size would be better. However, if Rust for Linux would prefer to use the automatic indexing approach I described in Support.rust-project.json(i.e. hidden) #17816 (comment), maybe that matters less?
Anyways, I think this is a reasonable addition; I just want to make sure that some details of my understanding are not out date.
| tracing::error!( | ||
| "Unknown cfg group `{cfg_extend}` in crate `{}`", | ||
| crate_data.display_name.as_deref().unwrap_or("<unknown>"), | ||
| ); | ||
| [].iter().cloned() |
There was a problem hiding this comment.
More of a large meta point, but I think it's be a good idea to us to consider using https://github.com/rust-lang/annotate-snippets-rs for rust-analyzer-specific errors.
There was a problem hiding this comment.
That seems tailored to CLI output, doesn't sound too useful for us
Yeah, if we end up using the I guess other projects could benefit from it, but it is true that the kernel is a bit of an outlier in terms of |
|
Gotcha, yeah. We can probably land this this largely as-is and revisit in a year if its carrying its weight. |
|
I can make the crates share the cfg in memory too (i.e. not expand them when constructing the crate graph). It is not even hard, but it means when checking cfgs we will have to check for parent groups too, which may have an impact on rust-analyzer performance. I don't really know how to benchmark this, and if it indeed regresses perf, it raises the question of whether it is worth regressing perf for everyone to please the few that have a large amount of shared cfgs. |
ChayimFriedman2
force-pushed
the
rust-project-cfg-group
branch
from
a6a55c9 to
2607c09
Compare
|
I wouldn't worry about the memory usage impact for this I think, the main compelling argument to me was the file size reduction. But indeed, if the discovery mechanism solves this problem then I'd rather get rid of this (as in im fine with us landing this temporarily but removing it once its not longer needed by the linux project |
|
Will merge this for now, if the linux project doesn't need this anymore in the future (due to project discovery etc) let us know then we'll likely remove it again |
|
☀️ Test successful - checks-actions |
Thanks! |
Closes #17815.