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

Deprecate cargo macros #2840

Open
2 tasks done
kbknapp opened this issue Oct 9, 2021 · 11 comments
Open
2 tasks done

Deprecate cargo macros #2840

kbknapp opened this issue Oct 9, 2021 · 11 comments
Labels
A-builder Area: Builder API C-bug Category: Updating dependencies E-easy Call for participation: Experience needed to fix: Easy / not much
Milestone

Comments

@kbknapp
Copy link
Member

kbknapp commented Oct 9, 2021

Maintainer's notes

Path forward

  1. Create a crate with these, and other related cargo env variables
  2. Port clap over to this crate
  3. Deprecate the crate_* macros, pointing people to that crate

Please complete the following tasks

  • I have searched the discussions
  • I have searched the existing issues

Clap Version

3.0.0-beta.4

Describe your use case

I'm proposing removing the convenience macros for getting information from cargo at build time. Specifically:

  • crate_license
  • crate_version
  • crate_authors
  • crate_description
  • crate_name

None of these utilize anything inside of clap and simply call out to checking environment variables that cargo populates at compile time.

Describe the solution you'd like

These would be prime candidates for spinning off into a crate dedicated to such information about Cargo. I'd actually be kind of surprised if a crate with this functionality doesn't already exist. If one doesn't exist, I'm not against hosting one inside the clap-rs org, but being that it's not actually clap related this repository is probably a bad fit.

I propose we deprecate for v3, and remove for v4. That gives us time to either find a crate to suggest or pull these macros into one ourselves.

Alternatives, if applicable

  • We keep these macros unchanged as they're small and self contained

Additional Context

I'm open to suggestions and thoughts on the matter.

@kbknapp kbknapp added T: RFC / question M-breaking-change Meta: Implementing or merging this will introduce a breaking change. labels Oct 9, 2021
@kbknapp kbknapp added this to the 3.0 milestone Oct 9, 2021
@pksunkara
Copy link
Member

pksunkara commented Oct 9, 2021

I would agree but we use them in app_from_crate! and as far as I understand from looking at people creating issues about this part of the code these are really widely used for most the non-popular clis.

@kbknapp
Copy link
Member Author

kbknapp commented Oct 9, 2021

app_from_crate! could use whatever crate these would get spun off into so I'm not too concerned from that angle.

I'm more worried that because these aren't clap related, what if people want more cargo metadata exposed? These started as just crate_version! but have since grown to a few more. Sure they're not big or complex, but I think they'd fit better into a crate dedicated to that functionality.

I also agree they're pretty widely used which I think adds support to us taking it slow if we move forward (deprecate for a while, remove in a future major release), and also adds support to us creating the crate in our org with an identical API to make the transition painless.

@pksunkara pksunkara changed the title Remove / Deprecate cargo macros Deprecate cargo macros Oct 9, 2021
@pksunkara pksunkara added C: other and removed T: RFC / question M-breaking-change Meta: Implementing or merging this will introduce a breaking change. labels Oct 9, 2021
@epage
Copy link
Member

epage commented Oct 9, 2021

I can understand the desire to spin things off. I've wanted arg_enum! in non-clap settings.

I agree that the right route forward is a crate we delegate to but still provide a convenience for clap users.

@epage
Copy link
Member

epage commented Oct 9, 2021

Since a deprecation is not a breaking change, should we move this out of the 3.0 milestone?

@pksunkara
Copy link
Member

It is in 3.0 milestone similar to #2835 because we need to add deprecation warnings.

@epage
Copy link
Member

epage commented Oct 9, 2021

We should not be deprecating anything without having a replacement to point people towards. Unless we are creating the crate now, we shouldn't do this.

With #2835, we have replacements (builder, derive, from usage) that we can point people to as the recommended approach.

@kbknapp
Copy link
Member Author

kbknapp commented Oct 10, 2021

Since a deprecation is not a breaking change, should we move this out of the 3.0 milestone?

Good point. My point was just to deprecate for 3.0 like @pksunkara mentioned, but you're correct we could deprecate anywhere in the 3.x lifetime. I'll place this at the 3.1 milestone as a placeholder for "decide on this sometime in 3.x"

And of course we're not locked into spinning these out, I'm open to suggestions and opinions on the matter too.

@kbknapp kbknapp modified the milestones: 3.0, 3.1 Oct 10, 2021
@joshtriplett
Copy link
Contributor

joshtriplett commented Oct 11, 2021

I think it'd be perfectly reasonable to just make these non-public before 3.0, as long as the top-level app_from_crate macro still exists.

I don't think we have to provide a replacement first. These macros just existed to pass to the appropriate clap functions.

@epage
Copy link
Member

epage commented Oct 11, 2021

I think it'd be perfectly reasonable to just make these non-public before 3.0, as long as the top-level app_from_crate macro still exists.

For context, I'm recommending in #2617 that we provide built-in transition paths for users via deprecations to streamline the upgrade process. We can do breaking changes but ideally we have a deprecation windows first

@pksunkara
Copy link
Member

I think it'd be perfectly reasonable to just make these non-public before 3.0, as long as the top-level app_from_crate macro still exists.

IIRC, people actually use those underlying macros directly instead of app_from_crate! and created some issues around them.

@epage epage added A-builder Area: Builder API C-bug Category: Updating dependencies E-easy Call for participation: Experience needed to fix: Easy / not much and removed C: other labels Dec 8, 2021
@epage
Copy link
Member

epage commented Feb 2, 2022

Random notes:

  • I recommend the crate-ci org for this
  • We should cross-link with build_script and shadow for other cargo related APIs
  • build-env and cargo-env names are taken. The next best I can think of is pkg-env.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-builder Area: Builder API C-bug Category: Updating dependencies E-easy Call for participation: Experience needed to fix: Easy / not much
Projects
None yet
Development

No branches or pull requests

4 participants