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

chore: please make unstable truly unstable #128

Closed
dzmitry-lahoda opened this issue Sep 11, 2024 · 18 comments
Closed

chore: please make unstable truly unstable #128

dzmitry-lahoda opened this issue Sep 11, 2024 · 18 comments

Comments

@dzmitry-lahoda
Copy link
Contributor

dzmitry-lahoda commented Sep 11, 2024

currently unstable tests on 2 years old version of rust, it is wrong. 2 years version of rust is not unstable anymore - i would say that version does not exist and testing on is dummy stub (so it tests nothing at all).

@Veetaha
Copy link
Contributor

Veetaha commented Sep 11, 2024

The test-unstable tests run against the latest beta and nightly versions. See the logs here. Could you elaborate?

@dzmitry-lahoda
Copy link
Contributor Author

image

@dzmitry-lahoda
Copy link
Contributor Author

dzmitry-lahoda commented Sep 11, 2024

rust-version = "1.59.0"

so bon-macro is tested with old version of rust, not nightly/beta.

so not sure why limit macro to old version (it runs on dev machine with for sure has compiler not 2 years old).

it is clear why bon crate limited so.

@Veetaha
Copy link
Contributor

Veetaha commented Sep 11, 2024

It's just a clippy lint called clippy::incompatible_msrv. The jobs still test against latest beta/nightly

@Veetaha
Copy link
Contributor

Veetaha commented Sep 11, 2024

The rust-version field in Cargo.toml declares the minimum required version of rustc for this library to compile. Given that people often upgrade their rust toolchain lazily libraries have to support older compiler versions if they want to support projects with older rustc versions.

Also Linux distros come with a preinstalled rust toolchain by default which is often quite outdated (#81 (comment))

@dzmitry-lahoda
Copy link
Contributor Author

dzmitry-lahoda commented Sep 12, 2024

For stable version fine to support old. No problem.
So still problem. Was Debian that Linux distro dicussed to have old version of Rust? Debian 11 LTS 1.48 rust. Next current Debian LTS is 1.63. Why not 1.48 than ? Because 1.59 will not compile right? Or 1.63? Or other target Linux system was used?

For unstable version, that weird. Considering 1.59 version to be unstable is weird. It does not makes any sense. Is there any prominent crate doing that?

@dzmitry-lahoda
Copy link
Contributor Author

dzmitry-lahoda commented Sep 12, 2024

I mean fine to compile stable version of bon with rust target to be 1.59(so may be 1.63 is more reasonable?). But compiling unstable version of bon with rust target 1.59 does not have any sense. Unless I miss something?

@Veetaha
Copy link
Contributor

Veetaha commented Sep 12, 2024

bon must always comply with 1.59 MSRV no matter what version of toolchain is used to be always be consistent (except for known exceptions marked with rustversion attribute). There is no exception for the unstable toolchain

@Veetaha
Copy link
Contributor

Veetaha commented Sep 12, 2024

I don't understand what change you propose, and why

@Veetaha
Copy link
Contributor

Veetaha commented Sep 12, 2024

I assume you confuse unstable toolchain and "unstable" cargo feature flag, which absolutely have nothing in common. The feature flag is just a marker for the user to sign the contract that bon may break the API of #[builder(getter)] attributes in patch releases. And my intention with that feature flag was to check for it's presence in the validation method of parameters before allowing the usage of builder(getter).

See my PR comment. I didn't ask to put #[cfg(feature = "unstable")] all over the implementation, I meant a simple

if !cfg!(feature = "unstable") && self.getter.is_some() {
    bail!(...)
}

#118 (review).

The getters feature must compile on all versions of rustc supported by bon because it'll eventually become stable. Anyone trying out that feature is not required to install the unstable rust toolchain

@Veetaha
Copy link
Contributor

Veetaha commented Sep 12, 2024

This is following the similar approach of clap https://github.com/clap-rs/clap/blob/df1efca03509f736e26d2f16766b7ea06acc3ecf/Cargo.toml#L169. I think it means the feature flag should be called unstable-getters, not just unstable or get to mark a specific feature as unstable

@dzmitry-lahoda
Copy link
Contributor Author

installed 1.59 rust, it fails

Updating crates.io index
error: failed to select a version for the requirement `syn = "^2.0.56"`
candidate versions found which didn't match: 2.0.56, 2.0.55, 2.0.54, ...
location searched: crates.io index
required by package `bon-macros v2.2.1 (/home/dz/github.com/elastio/bon/bon-macros)`

so not clear at all what currently checked, but for sure no that specific linuxes with old version of rust can use bon macro. they cannot use bon macro using rust compiler version of 1.59.

@Veetaha
Copy link
Contributor

Veetaha commented Sep 12, 2024

They can, they just need to use lower versions of dependencies. Take a look at how test-msrv.sh works:

bon/scripts/test-msrv.sh

Lines 35 to 38 in 3217b4b

step cargo update -p syn --precise 2.0.56
step cargo update -p tokio --precise 1.29.1
step cargo update -p expect-test --precise 1.4.1
step cargo update -p windows-sys --precise 0.52.0

@dzmitry-lahoda
Copy link
Contributor Author

I didn't ask to put #[cfg(feature = "unstable")] all over the implementation, I meant a simple
if !cfg!(feature = "unstable") && self.getter.is_some()

@Veetaha not sure I get that. So you propose to have attribute to be available to put by users, but when macro generates code it just ignores it? Is not that bad? I supposed if get does not work it should not be available to put as macro attribute by user?

@Veetaha
Copy link
Contributor

Veetaha commented Sep 12, 2024

just ignores it

No, the macro must return an error. See the bail!(...) inside of the if?

@dzmitry-lahoda
Copy link
Contributor Author

dzmitry-lahoda commented Sep 12, 2024

unstable-getters

why not singular getter? so attribute name equals feature? why it is unstable when it will be build on stable super old rust? or why it is unstable needed if it is behind feature getter as has one and only one impl possible (not so much can think for getter unless whole gen to be changed - but than whole bon is unstable)?

@dzmitry-lahoda
Copy link
Contributor Author

No, the macro must return an error. See the bail!(...) inside of the if?

would not rust suggest option to type that field and than error during compilation in this case? how to ensure there is not suggestion to user to type getter if feature is off?

@Veetaha
Copy link
Contributor

Veetaha commented Sep 12, 2024

There are no completions in IDEs for proc macro attributes (except for some manual hacks that I did, which are limited and don't cover member-level attributes).

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

No branches or pull requests

2 participants