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

Use $crate prepend to make importing the _clap_count_exprs macro unnecessary #1397

Merged
merged 2 commits into from
Mar 26, 2019
Merged

Use $crate prepend to make importing the _clap_count_exprs macro unnecessary #1397

merged 2 commits into from
Mar 26, 2019

Conversation

ErichDonGubler
Copy link
Contributor

@ErichDonGubler ErichDonGubler commented Dec 18, 2018

Resolves #1329, using the $crate:: keyword that was stabilized with Rust 1.30.0.

NOTE: This should be merged on or after the day of the Rust 1.32 (probably 2019-01-21, 6 weeks after 1.31.0 released) release in order to maintain the supports($LATEST - 2) minimum Rust compiler policy.

@hcpl
Copy link
Contributor

hcpl commented Dec 19, 2018

Could this fix use #[macro_export(local_inner_macros)] instead which is compatible with older Rusts?

@ErichDonGubler
Copy link
Contributor Author

ErichDonGubler commented Dec 19, 2018

@hcpl: Several wrapper macros would need to be generated for usages of several macros in std, but that might work. With this diff based on the current master:

diff --git a/src/macros.rs b/src/macros.rs
index 0f9e755c..1962c76a 100644
--- a/src/macros.rs
+++ b/src/macros.rs
@@ -303,7 +303,7 @@ macro_rules! _clap_count_exprs {
 /// [`Display`]: https://doc.rust-lang.org/std/fmt/trait.Display.html
 /// [`std::fmt::Display`]: https://doc.rust-lang.org/std/fmt/trait.Display.html
 /// [`Arg::case_insensitive(true)`]: ./struct.Arg.html#method.case_insensitive
-#[macro_export]
+#[macro_export(local_inner_macros)]
 macro_rules! arg_enum {
     (@as_item $($i:item)*) => ($($i)*);
     (@impls ( $($tts:tt)* ) -> ($e:ident, $($v:ident),+)) => {

...these are the macros that currently fail:

  • vec
  • stringify
  • format
  • write

My main concern is the sheer number of changes that fixing compile errors after applying this diff would entail -- there are 48 places where macro expansion fails after applying this diff. The solution using local_inner_macros could be used now, while this simpler one will be effective come mid-January. Given that the workaround for this issue is simple (adding a single import for _clap_count_exprs), I'm inclined to think that this issue isn't urgent enough to need local_inner_macros.

@hcpl
Copy link
Contributor

hcpl commented Dec 19, 2018

While you are right about boilerplate to work around external macro usages being cumbersome, I was more worried about using a latest Rust-only feature. This forces users to get the latest Rust to build clap at all which doesn't look like a nice user experience to me. And doubly so if they use something else than rustup to manage their Rust installations, like system package managers in distros, which likely lag behind rustup for some time or don't even provide new Rust versions at all if the package set for their distro is frozen (Debian testing and stable respectively come to mind).

So IMO taking the burden off from users (requiring them to have Rust 1.32+) to the library (maintaining local_inner_macros with wrappers of std macros) is worth it in the long run. As an example of a positive effect, they wouldn't have to face weird compilation errors related to minimum supported Rust, especially if they are new to the language.

@ErichDonGubler
Copy link
Contributor Author

Maybe-not-minor nitpick:

requiring them to have Rust 1.32+

That would technically be Rust 1.30+. :)

To your point: ack, I wasn't thinking about non-rustup installs. What are the earliest versions in common distribution right now? Are there any lower than Rust 1.30?

@ErichDonGubler
Copy link
Contributor Author

ErichDonGubler commented Dec 19, 2018

Debian's stable release channel uses Rust 1.24.0.

Fedora's oldest supported version is 28, and the oldest version of rustc on that is Rust 1.25.0.

Ubuntu's oldest supported version is Trusty (14.04), which uses Rust 1.28.

Gentoo's oldest supported Rust version is Rust 1.29.1.

openSUSE's oldest version of their rust package is Rust 1.24.0.

OpenBSD's oldest supported version is 6.2, and the oldest version I can find on the supported architectures is Rust 1.20.0

@ErichDonGubler
Copy link
Contributor Author

ErichDonGubler commented Dec 19, 2018

@hcpl: So, back to this point of yours:

I was more worried about using a latest Rust-only feature. This forces users to get the latest Rust to build clap at all which doesn't look like a nice user experience to me.

The policy of this crate is to support at most two versions BEFORE latest. Even the theoretically current minimum of Rust 1.29 would break every Linux/BSD distro I found above except Gentoo.

@hcpl
Copy link
Contributor

hcpl commented Dec 22, 2018

That would technically be Rust 1.30+. :)

Oh derp, I should have read more carefully :).

The policy of this crate is to support at most two versions BEFORE latest. Even the theoretically current minimum of Rust 1.29 would break every Linux/BSD distro I found above except Gentoo.

Yeah, I should have pointed out the "latest stable - 2" policy being explicitly mentioned in README in my comments, but oh well.

When writing comments above, I had the mindset of "even in presense of a lenient policy, don't break unless something external (e.g. dependencies) forces you because doing so is in good taste for users". But that's me being too conservative about not upsetting users which can be a bad thing (my policy would accumulate more technical debt over time, for example).

In the end, I'm totally fine with what you are doing here since it complies with the library policy and my point about doing a little more that you promised to is not applicable to everyone.

@killercup killercup mentioned this pull request Jan 3, 2019
@ErichDonGubler
Copy link
Contributor Author

ErichDonGubler commented Jan 23, 2019

Just wanted to ping this issue again, since Rust 1.32 has now released and this should be good to go!

@mkpankov
Copy link

mkpankov commented Feb 4, 2019

Wanted to say that PR apparently doesn't compile on nightly, but the fail is in libc, so all good actually.

@WaDelma
Copy link

WaDelma commented Mar 23, 2019

Is this PR going to be fixed/merged? I just stumbled into this in a new project and feel dirty using "private" macros.

@ErichDonGubler
Copy link
Contributor Author

I'm not sure. There's been radio silence the last couple of months... :\

@ErichDonGubler
Copy link
Contributor Author

ErichDonGubler commented Mar 26, 2019

Some discussion has been happening here on Reddit, initiated by @mkpankov: https://www.reddit.com/r/rust/comments/b575hi/is_clap_maintained/ejdawiv/

TL;DR: @kbknapp has been focusing the few hours per week he's had on clap 3.0. Some new maintainers have been added to the repo to help alleviate the pressure on PRs and issues. Hopefully they can get a look at this soon!

@spacekookie
Copy link
Contributor

@ErichDonGubler hey there, yea I'll try to get this today :)

Copy link
Contributor

@spacekookie spacekookie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@spacekookie spacekookie merged commit 9d31e63 into clap-rs:master Mar 26, 2019
@ErichDonGubler
Copy link
Contributor Author

Just wanted to give a shout-out to @kbknapp and @spacekookie for taking the time to make things transparent -- it took a while, but it was because life happens and other priorities do come up! I feel good about the outcome here, and I hope others do too. :)

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.

None yet

5 participants