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

Reduce clap2->clap3 porting burden #2617

Closed
2 tasks done
epage opened this issue Jul 22, 2021 · 18 comments
Closed
2 tasks done

Reduce clap2->clap3 porting burden #2617

epage opened this issue Jul 22, 2021 · 18 comments
Labels
C-bug Category: Updating dependencies
Milestone

Comments

@epage
Copy link
Member

epage commented Jul 22, 2021

Please complete the following tasks

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

Rust Version

rustc 1.53.0 (53cb7b09b 2021-06-17)

Clap Version

3.0.0-beta.2

Minimal reproducible code

See https://github.com/rust-cli/argparse-benchmarks-rs/blob/main/examples/clap-app/app.rs

Steps to reproduce the bug with the above code

Port it to clap3

Actual Behaviour

Have to dig through the docs and changelog where the more that exists, the harder to find content

We especially don't want noise from mechanical transitions to drown out behavior changes like #751 or removal of implied settings

Expected Behaviour

Deprecation warnings

Additional Context

Where its trivial, I'd propose we add functions back in

  • Forward to new impl
  • Have deprecation attribute
  • Doc comment is only brief enough to mention what should be used

Examples:

  • Arg::multiple
  • Arg::with_name
  • Arg::help

Debug Output

No response

@epage epage added the C-bug Category: Updating dependencies label Jul 22, 2021
@ducaale
Copy link
Contributor

ducaale commented Jul 24, 2021

I think clap_up can be used to automate the upgrade to v3.

@epage
Copy link
Member Author

epage commented Jul 24, 2021

Yes, I've seen clap_up. Its unclear whats its state is since it doesn't even compile at the moment.

@kornelski
Copy link
Contributor

kornelski commented Jul 25, 2021

If you don't want to start 3.0 with deprecated functions, then I suggest doing the migration on 2.x: backport as much as possible, add methods under new names in 2.x, and then deprecate old names in 2.x.

@pksunkara pksunkara added this to the 3.0 milestone Jul 27, 2021
@pksunkara
Copy link
Member

I have built the https://github.com/pksunkara/cargo-up using the efforts of https://github.com/rust-analyzer/rust-analyzer which can be used to port all kinds of code from clap v2 to v3.

I'd propose we add functions back in

I am of the strict opinion that we don't want to do this. The field of programming is so advanced right now that it should be trivial to automatically upgrade the code when upgrading dependencies.

The clap_up code is pretty good right now (upgrades almost all of ripgrep). Need more polish, need to handle features though. I was just waiting to do clap 3.0 rc release before concentrating on finishing the upgrader.

@epage
Copy link
Member Author

epage commented Jul 27, 2021

I am of the strict opinion that we don't want to do this. The field of programming is so advanced right now that it should be trivial to automatically upgrade the code when upgrading dependencies.

Programming might be, but we still have to solve the human problem:

  • Some won't have read the docs and know about the tool and this can serve as a way to advertise it.
  • Others might find installing a one-off tool a hassle, so giving them inline guidance makes things easier

(I suspect I'd fall into both camps)

And as I said, clap_up doesn't even compile atm and we've not been making sure we update it (and the CHANGELOG) in PRs.

Personally, I feel like clap_up adds risk of more delay to getting clap3 out. This has taken years already; let's focus on what will get this into people's hands rather than trying for perfection.

@pksunkara
Copy link
Member

Let me ask a simple question (to make my argument), what are the kind of changes we can add deprecation notice for?

My answer would be simple code replacements for functions. Are there more?

@epage
Copy link
Member Author

epage commented Oct 1, 2021

Having me play guessing games with what your point is is not constructive to the conversation and slows it down dramatically due to the delays in responses as we do back and forths. Please just make your point.

@pksunkara
Copy link
Member

No, I am just trying to understand what all the kind of deprecations are possible in Rust. That was the only case deprecations are made for, right? I am asking you if I am missing more.

@epage
Copy link
Member Author

epage commented Oct 1, 2021

No, I am just trying to understand what all the kind of deprecations are possible in Rust.

As for what can be marked deprecated, I believe you can attach it to any item (module, function, etc) and on use of that, users will get a warning.

As a heads up, by framing it with "to make my argument", it implies this is a leading question to a point you are trying to make which is what I was responding to.

@pksunkara
Copy link
Member

Yeah, I probably misphrased my intention. What I meant is that I need some more info before I make my argument clear.

I believe you can attach it to any item (module, function, etc) and on use of that, users will get a warning.

Yeah, but all of them would need the code inside them to be replaced to work with new broken API, right? It's not like we can mark something as deprecated but not change it if the internals change.

@epage
Copy link
Member Author

epage commented Oct 1, 2021

We'd need to make sure the function doesn't have a significant breakage of functionality. If that means we need to provide code that adapts, that is one way of addressing it. For when we change a default, we just might make the function or enum a no-op.

epage added a commit to epage/clap that referenced this issue Oct 1, 2021
@kbknapp
Copy link
Member

kbknapp commented Oct 2, 2021

I know I pop in and give historical context in almost all my comments...I apologize.

However, my original intention (right, wrong, or indifferent) was to have v3 at RC and then go back through v2 marking everything that is missing or changed and make a final v2 release with all the deprecations.

There are some APIs from v2 that don't need to be removed in v3 and can just maintain the deprecation warning (I.e. this will be removed in v4, etc.). Other APIs cannot co-exist in v3 because perhaps the underlying meaning changed/was removed, etc.

So our challenge is both to identify both of those types of deprecations. For the ones that cannot co-exist in v3, we must clearly mark those as "removed in v3", for all others can can co-exist in v3 but remain deprecated to ease transition we mark them as "deprecated, use XYZ instead; will be removed in v4"

Most of this is my fault for delaying v3 so long and trying to get so much into it, while also trying to maintain v2 as a one man show. This led to not being able to put proper deprecation notices on v2 APIs because I wasn't sure on the solid v3 API yet.

As for clap_up I'm very much in favor of using it, but not relying on it. I.e. I love that it's an option, I definitely want to advertise it as the go-to option for people if they're willing, but it absolutely shouldn't be the only option in lieu of good documentation, deprecation notices, and trying to ease the pain of upgrading as much as possible through good design choices. I work in an environment where it probably wouldn't be an option just by policy, and I imagine I'm not the only one out there. Likewise some people are cautious with automated upgrades because unless there is 100% certainty and test coverage subtle changes can sneak in.

Also, I'm aware that by saying "some of those good design choices" may mean kicking the can, for certain changes, down the road until v4+ in order to ease transition.

@epage
Copy link
Member Author

epage commented Oct 2, 2021

clap_up seems like it'll be really helpful for those able to and up for running it!

I do have some questions though for our plan moving forward:

  • Do we gate v3 on clap_up?
  • What is the expected quality and completeness bar for clap_up and how do we communicate that if its not 100%?
  • How much do we let it impact other development (compile and test times; it pulls in some pretty heavy dependencies(?

Also, I'm aware that by saying "some of those good design choices" may mean kicking the can, for certain changes, down the road until v4+ in order to ease transition.

Yeah, I fully support pushing back to v4 or later several things I'm really interested in to stagger upgrade pain (using ranges for number of values, adjusting what Arg::new does, etc).

epage added a commit to epage/clap that referenced this issue Oct 4, 2021
- `App::with_defaults` was not included since that has been deprecated
  since 2.14
- `App::args_from_usage` does not have a close enough parallel in the
  new API, as far as I could tell
- `ArgMatches::usage` cannot have a thin wrapper around
  `App::generate_usage`.
- `App::write_*`: getting lazy, didn't seem like high value functions
- Any `Settings` (some things need to be figured out here)

This is a part of clap-rs#2617
epage added a commit to epage/clap that referenced this issue Oct 7, 2021
- `App::with_defaults` was not included since that has been deprecated
  since 2.14
- `App::args_from_usage` does not have a close enough parallel in the
  new API, as far as I could tell
- `ArgMatches::usage` cannot have a thin wrapper around
  `App::generate_usage`.
- `App::write_*`: getting lazy, didn't seem like high value functions
- Any `Settings` (some things need to be figured out here)

This is a part of clap-rs#2617
epage added a commit to epage/clap that referenced this issue Oct 7, 2021
- `App::with_defaults` was not included since that has been deprecated
  since 2.14
- `App::args_from_usage` does not have a close enough parallel in the
  new API, as far as I could tell
- `ArgMatches::usage` cannot have a thin wrapper around
  `App::generate_usage`.
- `App::write_*`: getting lazy, didn't seem like high value functions
- Any `Settings` (some things need to be figured out here)

This is a part of clap-rs#2617
epage added a commit to epage/clap that referenced this issue Oct 11, 2021
A lot of users expected `color` feature flag and `ColorAuto` etc to
control all colors.  Having this extra flag around is easy to miss and
adds to our overall settings bloat, making it harder to find settings
people want.

This completely removes it, rather than make it deprecated like
functions in clap-rs#2617, because there is extra work to mark things
deprecated as Settings and we should decide on our strategy first before
investing time in addressing that issue.

Fixes clap-rs#2806
epage added a commit to epage/clap that referenced this issue Oct 12, 2021
- `App::with_defaults` was not included since that has been deprecated
  since 2.14
- `App::args_from_usage` does not have a close enough parallel in the
  new API, as far as I could tell
- `ArgMatches::usage` cannot have a thin wrapper around
  `App::generate_usage`.
- `App::write_*`: getting lazy, didn't seem like high value functions
- Any `Settings` (some things need to be figured out here)

This is a part of clap-rs#2617
epage added a commit to epage/clap that referenced this issue Oct 15, 2021
- `App::with_defaults` was not included since that has been deprecated
  since 2.14
- `App::args_from_usage` does not have a close enough parallel in the
  new API, as far as I could tell
- `ArgMatches::usage` cannot have a thin wrapper around
  `App::generate_usage`.
- `App::write_*`: getting lazy, didn't seem like high value functions
- Any `Settings` (some things need to be figured out here)

This is a part of clap-rs#2617
@pksunkara
Copy link
Member

pksunkara commented Oct 18, 2021

Let's talk about types of breaking changes.

  1. APIs that can co-exist.

    These are the ones that are from the old code but can live as new code. These are the ones that are added back in fix: Ease clap2->clap3 migration with deprecations #2718. A good example is a renamed method.

    In clap v2:

    let arg = Arg::new("name").required_if("other", "value");

    In clap v3:

    let arg = Arg::new("name").required_if_eq("other", "value");

    These type of API changes are easy to migrate by adding deprecation notices and implementing the new functionality in the old API.

    fn required_if(mut self, key: &str, value: &str) -> Self {
        self.required_if_eq(key, value);
    }
  2. APIs that can not co-exist.

    These are the ones that have their behaviour changed and cannot exist alongside the new API due to various reasons. A good example is a behaviour change.

    In clap v2:

    let arg = Arg::new("name").require_equals(true);

    In clap v3:

    let arg = Arg::new("name").require_equals(true).forbid_empty_values(true);

    These type of API changes cannot be marked with deprecation notices at all.


Now, if a new version of a library contains only the first type of breaking changes, it is easy to mark them as deprecated in the new version and give users an easy way to migrate to the new version of the library.

But if that new version of the library also contains the above mentioned second type of breaking changes, the end user would need to go through the library changelog to migrate to the new version of the library.

In this second scenario, while I do agree that deprecation notices do help, I have never seen anyone deciding to keep using the old API when they are actually migrating to a new version of the library by trawling through the changelog.

It is actually harder for the end user to actually recognise there are some behaviour breaking changes without reading the changelog since these second type of breaking changes do not have deprecation notices.

Not only that, but keeping this old and dead code also makes the library hard to maintain. It also confuses new users of the library by adding unnecessary additions to the documentation.

Thus it is of my opinion (and this is something I strongly feel for) that it would be much more beneficial if the new API is a clean slate in the second scenario and develop tools to migrate the library. Which is the main reason behind why I started cargo-up.


Of course, I am not completely disregarding the people who want to manually migrate and also want deprecation notices. But my question here is, why do they need the deprecation notices in the new version of the library? Can't we release another patch (or minor) to the old version of the library with the deprecation notices added?

This opinion has been expressed by other people above too:

then I suggest doing the migration on 2.x: backport as much as possible, add methods under new names in 2.x, and then deprecate old names in 2.x.

However, my original intention (right, wrong, or indifferent) was to have v3 at RC and then go back through v2 marking everything that is missing or changed and make a final v2 release with all the deprecations.

And this method not only keeps the v3 API and docs clean, but also helps people understand what all the breaking changes they need to migrate by adding deprecation notices even for behaviour changes in the old API. Thus it is easy for the user to immediately know which API to migrate even in the case of the second type of breaking changes (especially behaviour changes).

This kind of release can be termed as a Deprecation release. We don't need to have alternatives for deprecated API at all in the same release because it is purely a warning release and users still have the option to pin the library to a previous compatible release.


tl;dr: Since we have behaviour changes, it is much better to do a warning release in v2 and keep the API clean for v3


Do we gate v3 on clap_up?

No.

What is the expected quality and completeness bar for clap_up and how do we communicate that if its not 100%?

It can print it's own warnings while pointing to the exact line and file in case there's something we could not complete.

How much do we let it impact other development (compile and test times; it pulls in some pretty heavy dependencies(?

It doesn't impact anything at all since it is not in default_members

@epage
Copy link
Member Author

epage commented Oct 18, 2021

There is a lot here. My quick summary of thoughts

  • Perfection is the enemy of good enough. Just because we can't handle all cases doesn't mean we shouldn't try
    • Yes, the changelog will still be needed but this makes the number of required changes to make smaller and it makes it so we can better focus the document so people don't miss a critical behavior change due to a mass of mechanical changes that deprecations could handle
  • clap3 is fairly different because of how big and long delayed of a release it is.
    • Ideally, we would have been making as many of these changes during clap2 with depredations and feature gates.
    • Updating clap2 has a lot more limited of a lifetime compared to having an active series of releases to migrate

What is the expected quality and completeness bar for clap_up and how do we communicate that if its not 100%?

It can print it's own warnings while pointing to the exact line and file in case there's something we could not complete.

I'm referring to level of compeleteness / user-facing quality.

How much do we let it impact other development (compile and test times; it pulls in some pretty heavy dependencies(?

It doesn't impact anything at all since it is not in default_members

I had assumed the intent was for it eventually to be a default member or otherwise/. No matter what direction we go, its a burden

  • Not a default member, no CI:
    • it dies from entropy
    • Extra work is needed for "faster" cargo test times to only test clap
  • Not a default member, CI:
    • people are held accountable to it but it isn't included in basuic workflow
    • Extra work is needed for "faster" cargo test times to only test clap
    • Slow CI
  • We remove default-members and use --workspace
    • Faster cargo test in general cases
    • Still a hassle with --workspace and general CI times

If we are leaving it out of default-members, should it even be in the workspace and in the repo?

epage added a commit to epage/clap that referenced this issue Oct 18, 2021
This brings back the old name of settings, just deprecated.  Since they
all map to the same bits in the bit field, this should work for
`setting` and `is_set`.  The only thing this lacks is being able to do
equality across variants, whcih seems like a minority case.

Removed settings have some extra care abouts that we'll need to look
into separately.

This is a part of clap-rs#2617
epage added a commit to epage/clap that referenced this issue Oct 18, 2021
This partiall reverts commit efeb02c,
bringing back the `AppSettins::Color*` and making them the backing store
for `App::color`, to help ease the transition from clap2->3.

Once we remove these deprecated settings, we might want to keep this
backing store to save on memory.

This is a part of clap-rs#2617
@pksunkara
Copy link
Member

pksunkara commented Oct 18, 2021

Perfection is the enemy of good enough. Just because we can't handle all cases doesn't mean we shouldn't try

That's old thinking. We are not bogged down by CPU power anymore. We are not constrained by system memory anymore. We can actually run complex migrators now on normal computers.

Why do we want to settle for being "good enough" when we have the capability to reach for perfection? Where's the innovation? Where's the zeal for change? Where's the passion for the future? Where are the people who are willing to go beyond than normal?

I will tell you where. They are busy reading changelogs for non-coexistable changes to migrate their code to new dependency versions. They are busy working around bugs of a library they use because they don't have time to migrate it. They are busy maintaining inefficient code in libraries to keep it backward compatible. They are busy managing tech-debt in their libraries because of that inefficient code.


Not a default member, CI:
people are held accountable to it but it isn't included in basuic workflow
Extra work is needed for "faster" cargo test times to only test clap
Slow CI

It is basically tested only for releases. It's not like it will be in active development all the time. It's just a migration rules script.

@epage
Copy link
Member Author

epage commented Oct 19, 2021

Having a tool to automatically handle upgrades would be great! However, being able to rely on it exclusively requires solving problems like

  • Being trustworthy. There is a lot of mistrust for automatic code changes, which a lot of this comes back to transparency
  • Being transparent. This isn't just a tooling problem but both a retraining problem and maintaining someone's familiarity with their code. If someone doesn't feel like they understand what or why a change happened, how the new API works, that leads to a lack of trust in the tool and a hurdle for people doing any manual adjustments
  • Being fast. The slower it is, the less likely someone is willing to take a risk on it
  • Being complete. If we bail out and leave someone half-way through the conversion, especially if the conversion is not transparent, the person is unprepared for finishing the conversion.
  • Being low effort for crate authors to write. There is a trade off between a maintainers time and the communities. While there is a whole lot of community, multiplying costs, there is a bottleneck with maintainers. The more high value automatic upgrades, I presume, will come at a greater cost to write.
  • Being low burden on the crate authors to maintain. For example, I've previously enumerated problems with us keeping clap_up inside the workspace / repo. Its dead code, weighing us down.

Without solving these human problems, we've invested a lot in a tool people likely won't use. And end-user adoption isn't binary, all or nothing, but a continuum, depending on how well the human problems are solved. Even if its good enough and we keep a focus on clap_up, deprecating still helps us with those who won't use it.

Also, I can understand exploring and stretching the expectations for the programming community but this project's focus is on command-line argument parsers, and not dependency upgrades. The clap community should not be taking on the early-adopter burden but weighing when cargo-up is ready and adopting it then. This is different than exploring tools like clog or cargo workspace with clap because any burdens are restricted to just the person doing releases, rather than impacting the whole clap community.

It is basically tested only for releases. It's not like it will be in active development all the time. It's just a migration rules script.

It sounds like its dead code, weighing down the project. Why don't we delete and it consider re-adding it later if we need it?

epage added a commit to epage/clap that referenced this issue Oct 23, 2021
This brings back the old name of settings, just deprecated.  Since they
all map to the same bits in the bit field, this should work for
`setting` and `is_set`.  The only thing this lacks is being able to do
equality across variants, whcih seems like a minority case.

Removed settings have some extra care abouts that we'll need to look
into separately.

This is a part of clap-rs#2617
epage added a commit to epage/clap that referenced this issue Oct 23, 2021
This partiall reverts commit efeb02c,
bringing back the `AppSettins::Color*` and making them the backing store
for `App::color`, to help ease the transition from clap2->3.

Once we remove these deprecated settings, we might want to keep this
backing store to save on memory.

This is a part of clap-rs#2617
@pksunkara
Copy link
Member

pksunkara commented Oct 24, 2021

However, being able to rely on it exclusively requires solving problems like

I completely agree with all those problems.

Let me clarify what I was proposing in my previous comment. Having a migrator is not a blocker but a helper. The main point of that proposal is the following:

Since we have behaviour changes, it is much better to do a warning release in v2 and keep the API clean for v3

It doesn't matter if there's a fully completed migrator or not. It is a related discussion only because of cargo fix vs cargo up. But that's just replacing text for API changes that are co-existable which is fully complete implementation wise. If you want, we can go more into pros and cons about how cargo fix relies on our API not being clean causing more confusion to new users.

epage added a commit to epage/clap that referenced this issue Oct 25, 2021
This brings back the old name of settings, just deprecated.  Since they
all map to the same bits in the bit field, this should work for
`setting` and `is_set`.  The only thing this lacks is being able to do
equality across variants, whcih seems like a minority case.

Removed settings have some extra care abouts that we'll need to look
into separately.

This is a part of clap-rs#2617
epage added a commit to epage/clap that referenced this issue Oct 25, 2021
This partiall reverts commit efeb02c,
bringing back the `AppSettins::Color*` and making them the backing store
for `App::color`, to help ease the transition from clap2->3.

Once we remove these deprecated settings, we might want to keep this
backing store to save on memory.

This is a part of clap-rs#2617
epage added a commit to epage/clap that referenced this issue Oct 26, 2021
This brings back the old name of settings, just deprecated.  Since they
all map to the same bits in the bit field, this should work for
`setting` and `is_set`.  The only thing this lacks is being able to do
equality across variants, whcih seems like a minority case.

Removed settings have some extra care abouts that we'll need to look
into separately.

This is a part of clap-rs#2617
epage added a commit to epage/clap that referenced this issue Oct 26, 2021
This partiall reverts commit efeb02c,
bringing back the `AppSettins::Color*` and making them the backing store
for `App::color`, to help ease the transition from clap2->3.

Once we remove these deprecated settings, we might want to keep this
backing store to save on memory.

This is a part of clap-rs#2617
epage added a commit to epage/clap that referenced this issue Nov 2, 2021
This brings back the old name of settings, just deprecated.  Since they
all map to the same bits in the bit field, this should work for
`setting` and `is_set`.  The only thing this lacks is being able to do
equality across variants, whcih seems like a minority case.

Removed settings have some extra care abouts that we'll need to look
into separately.

This is a part of clap-rs#2617
epage added a commit to epage/clap that referenced this issue Nov 2, 2021
This partiall reverts commit efeb02c,
bringing back the `AppSettins::Color*` and making them the backing store
for `App::color`, to help ease the transition from clap2->3.

Once we remove these deprecated settings, we might want to keep this
backing store to save on memory.

This is a part of clap-rs#2617
@epage epage closed this as completed Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Updating dependencies
Projects
None yet
Development

No branches or pull requests

5 participants