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

export core::cfg_if #479

Closed
lolbinarycat opened this issue Nov 11, 2024 · 14 comments
Closed

export core::cfg_if #479

lolbinarycat opened this issue Nov 11, 2024 · 14 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@lolbinarycat
Copy link

Proposal

Problem statement

cfg-if is one of the most widely used macro crates. it is as ubiquitous as it is simple, and it is also essentially "done", staying at 1.0.0 for 4 years.

however, external dependencies are never free, adding an extra http request to check for the latest version, an extra rustc invocation, as well as complicating the build process for projects that don't use cargo.

Motivating examples or use cases

the core library actually uses cfg_if 15 times, and the standard library uses it 65 times. however, because core is not allowed to have dependencies, this means that they are actually using separate, (but identical) macros.

Solution sketch

make the copy of cfg_if in core the definitive version, and make std depend on that.

Alternatives

  • add it to the std facade, and add it as a seperate sysroot crate.
  • wait for cfg_match to be stablized

Links and related work

https://crates.io/crates/cfg-if

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@lolbinarycat lolbinarycat added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Nov 11, 2024
@ChrisDenton
Copy link
Member

This is more libs than libs-api as there's no reason for this to ever be stabilized. Tbh I do think it better to move to cfg_match unless it's really not ready for use (and if that's the case it would be nice to find some people interested in working on it).

@lolbinarycat
Copy link
Author

Tbh I only found out about the existance of cfg_match about halfway through writing this.

still, even if it was stuck behind a perma-unstable attribute, this would basically give a free speedup to the process of building the standard library, which could reduce CI load a bit.

@kennytm
Copy link
Member

kennytm commented Nov 11, 2024

even if cfg_if is made public you still need to wait for it to stabilize, so the situation is not better than cfg_match.

is there anything cfg_if can do but cfg_match can't?

@the8472
Copy link
Member

the8472 commented Nov 11, 2024

however, external dependencies are never free, adding an extra http request to check for the latest version, an extra rustc invocation, as well as complicating the build process for projects that don't use cargo.

This isn't a good argument. Following it would mean we'd import all of crates.io into std.

@lolbinarycat
Copy link
Author

This isn't a good argument. Following it would mean we'd import all of crates.io into std.

The difference is cfg_if is already in the standard libraries, and it's also incredibly tiny, and unlikely to change.

I feel like I layed it out all the reasons this "put this in stdlib" request is different from most.

even if cfg_if is made public you still need to wait for it to stabilize

std wouldn't.

is there anything cfg_if can do but cfg_match can't?

I mean, the main advantage is cfg_if is already used in a ton of code...

@ChrisDenton
Copy link
Member

ChrisDenton commented Nov 11, 2024

even if cfg_if is made public you still need to wait for it to stabilize

std wouldn't.

We could just copy/paste the cfg_if crate into library/ and depend on it, using #[path] if it's important to avoid any extra rustc calls. Though again, I'd much rather try to use match_cfg.

Tbh, I'm not really sure if one small dependency in amongst many has that big an impact. If you don't have cargo you still have to have some way to compile std's other dependencies, no? And these dependencies are not stable. They can change depending on platform and over time.

@lolbinarycat
Copy link
Author

We could just copy/paste the cfg_if crate into library/ and depend on it

Are you missing the part where core already does that?

It's not about it being a huge improvement, it's about it having basically no downside as far as I can see. the only code that needs to be added is two attributes (#[macro_export] and a stability attribute), and you get to just remove a dependency from std.

@ChrisDenton
Copy link
Member

Are you missing the part where core already does that?

No, core pastes it into it's own macro file. I was suggesting putting it somewhere both core and std can depend on it independently. That would avoid the need to add an API.

It's not about it being a huge improvement, it's about it having basically no downside as far as I can see. the only code that needs to be added is two attributes (#[macro_export] and a stability attribute), and you get to just remove a dependency from std.

If we did it this way, I'd also prefer to make it #[doc(hidden)] to emphasise that it's not intended as a public feature.

@lolbinarycat
Copy link
Author

If we did it this way, I'd also prefer to make it #[doc(hidden)] to emphasise that it's not intended as a public feature.

yeah that's fine, most perma-unstable things are.

@ChrisDenton
Copy link
Member

I guess I'm very confused by this being an API Change Proposal

@lolbinarycat
Copy link
Author

my original intent was that it should be stabilized eventually, but you quickly convinced me it would be better off perma-unstable.

@cuviper
Copy link
Member

cuviper commented Nov 11, 2024

+1 for converting to cfg_match!

@kennytm
Copy link
Member

kennytm commented Nov 12, 2024

if this thing is going to be perma-unstable plus #[doc(hidden)] meaning it is just an implementation detail i don't see any reason keeping it as an ACP. a PR can just be filed directly?

that said i'd prefer migrating all 80 instances of cfg_if to cfg_match i.e. restarting rust-lang/rust#116342, rather than privately exporting core::cfg_if for std.

@Amanieu
Copy link
Member

Amanieu commented Nov 12, 2024

We already have cfg_match! as an unstable feature in the standard library. As such the team feels like there isn't a strong case for adding cfg_if and efforts should instead focus on moving cfg_match! forward.

@Amanieu Amanieu closed this as completed Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

6 participants