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

prqlc CLI and prqlc other than CLI should be separate crates or, the cli feature of prqlc should not be the default #4946

Open
eitsupi opened this issue Oct 10, 2024 · 6 comments

Comments

@eitsupi
Copy link
Member

eitsupi commented Oct 10, 2024

MSRV will become increasingly important as cargo will soon recognize the rust-version field and resolve dependencies.

Currently the prqlc crate is a required dependency of downstream packages that want to incorporate PRQL, but the MSRV of prqlc itself is pushed up by the MSRV of the cli feature dependency, which is not needed downstream.

MSRVs are not configurable on a per-feature basis, so as long as prqlc has the cli feature, prqlc's MSRVs are at risk of being raised by the cli feature.
This is especially troublesome for maintaining MSRV because the clap crate is not very conservative about MSRV (https://docs.rs/clap/latest/clap/#aspirations).

I think we should choose one of the following options to improve the situation here:

  1. Split prqlc into two crates, CLI and other.
    1. prqlc-cli and prqlc
    2. prqlc and prqlc-core (or something else)
  2. Remove the cli feature from prqlc default features. And ignore the MSRV of the cli feature.
@max-sixty
Copy link
Member

We've gone back & forth on this a few times on this! I agree there are tradeoffs.

It is a nicer more consistent interface to have cargo install prqlc, given we use prqlc for brew install prqlc / pip install prqlc / etc. Having one of them be prqlc-cli is not a disaster, but having really simple consistent interfaces is nice.

The main tradeoffs are that consuming libraries need to have default_features=false, and the MSRV issue. I would favor having a lower MSRV, especially since the MSRV resolver will make this much easier. I'm not confident on the costs of requiring libraries to remove default features; though it should be much less frequent than people installing the CLI...

@eitsupi
Copy link
Member Author

eitsupi commented Oct 12, 2024

It is a nicer more consistent interface to have cargo install prqlc, given we use prqlc for brew install prqlc / pip install prqlc / etc. Having one of them be prqlc-cli is not a disaster, but having really simple consistent interfaces is nice.

The main tradeoffs are that consuming libraries need to have default_features=false, and the MSRV issue. I would favor having a lower MSRV, especially since the MSRV resolver will make this much easier. I'm not confident on the costs of requiring libraries to remove default features; though it should be much less frequent than people installing the CLI...

If so, is there any reason against publishing the CLI as prqlc on crates.io and the compiler body as something like prqlc-core on crates.io?

@max-sixty
Copy link
Member

If so, is there any reason against publishing the CLI as prqlc on crates.io and the compiler body as something like prqlc-core on crates.io?

This would be possible; e.g. prqlc-lib, but what would the benefit over default-features=false be, assuming we can align the MSRVs?

@eitsupi
Copy link
Member Author

eitsupi commented Oct 12, 2024

This would be possible; e.g. prqlc-lib, but what would the benefit over default-features=false be, assuming we can align the MSRVs?

Wouldn't the CLI functionality be degraded because CLI dependencies, such as clap, would not be kept up to date?

@eitsupi
Copy link
Member Author

eitsupi commented Oct 12, 2024

Seems related to rust-lang/rfcs#3383

If the CLI can be squeezed into a feature, it means that all crates used within the CLI will be impossible to update due to restrictions imposed by non-CLI MSRVs (and version upgrades of their dependencies will be rejected because they do not justify the increase in MSRVs).
If that is acceptable, it certainly seems fine in its current state.

@max-sixty
Copy link
Member

Yes I definitely agree that's the tradeoff — if we find ourselves wanting a much newer MSRV than what we're tracking — currently we're about a year behind latest — then we should make a different crate. Otherwise it's simpler to integrate them.

Is that what you're thinking?


Nice find! I agree if that were implemented — particularly an auto-install per this comment — then splitting it off has fewer tradeoffs

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

No branches or pull requests

2 participants