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

Make cargo-platform optional #263

Closed
wants to merge 1 commit into from
Closed

Conversation

Jake-Shadle
Copy link
Contributor

@Jake-Shadle Jake-Shadle commented Jun 24, 2024

This makes cargo-platform optional.

When deserializing, the original string is completely lost which can be problematic if the original string is desired, rather than the "prettified" version, and also means that an additional heap allocation is required if using a different cfg parser like https://crates.io/crates/cfg-expr.

I've added it as the platform feature and made it non-default so that users need to opt-in if they still want to use cargo-platform, rather than making it default and then needing every crate that doesn't use it to require an opt-out.

@taiki-e
Copy link
Contributor

taiki-e commented Jun 24, 2024

I believe this has the same issue as #194.

@oli-obk
Copy link
Owner

oli-obk commented Jun 24, 2024

Doesn't the cargo-platform crate allow you to re-create the string (worst case serializing it will give it to you)?

If not, I think this should be pursued upstream. I would also not worry about a few extra allocations, as we already got a billion allocations all over the place. cargo-metadata should never be on a hot path where that would be noticeable

@Jake-Shadle
Copy link
Contributor Author

It does, but it loses the original information and can't give back the exact string, eg. cfg(target_os=\"linux\") will give back cfg(target_os = "linux") which makes it extremely annoying if you are trying to map a cargo_metadata target to the original span in the cargo manifest for the package. But yah, this would have similar issues to #194 so I'll just close this.

@Jake-Shadle
Copy link
Contributor Author

Actually just found this doesn't matter at all, cargo metadata itself prettifies the cfg expressions before emitting the json so there is no way to directly map regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants