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

Modernize crate #132

Merged
merged 5 commits into from
Nov 20, 2023
Merged

Modernize crate #132

merged 5 commits into from
Nov 20, 2023

Conversation

Emilgardis
Copy link
Member

No description provided.

@Emilgardis Emilgardis requested a review from a team as a code owner February 3, 2023 18:48
Comment on lines 162 to 160
.version(env!("CARGO_PKG_VERSION"))
.settings(&[
AppSettings::UnifiedHelpMessage,
AppSettings::DeriveDisplayOrder,
AppSettings::DontCollapseArgsInUsage,
])
// as this is used as a Cargo subcommand the first argument will be the name of the binary
Copy link
Member Author

Choose a reason for hiding this comment

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

These are all default now

README.md Outdated Show resolved Hide resolved
Comment on lines -1 to -2
#![deny(warnings)]

Copy link
Member Author

Choose a reason for hiding this comment

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

deny in ci instead

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
.multiple(true)
.number_of_values(1)
.takes_value(true)
.short('F')
Copy link
Member Author

Choose a reason for hiding this comment

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

a little bonus, -F was added in 1.62.0

burrbull
burrbull previously approved these changes Feb 3, 2023
@Emilgardis
Copy link
Member Author

Emilgardis commented Feb 4, 2023

Can we get some more eyes on this, don't want to accidentally break anything

I've tried to find usecases which don't work anymore but can't find any.

@Emilgardis
Copy link
Member Author

ping @rust-embedded/tools

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@Emilgardis
Copy link
Member Author

Emilgardis commented Sep 21, 2023

I'm confused, is actions/checkout@v4 now merging pull_request head with base? https://github.com/rust-embedded/cargo-binutils/actions/runs/6265523508/job/17014546817?pr=132#step:2:77

::edit:: apparently it's been this way for atleast 4 years now, I had no clue :)

@Emilgardis Emilgardis force-pushed the modernize branch 2 times, most recently from 59eb3d5 to 50bf217 Compare September 21, 2023 18:38
.github/workflows/release.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
also adds `-F` shorthand
@jonathanpallant
Copy link

What's the reason for moving the MSRV to 1.72.1? Debian is shipping 1.70.

@Emilgardis
Copy link
Member Author

Emilgardis commented Sep 25, 2023

The reason is because we can, and we dont have a policy except that we may bump the msrv whenever according to https://github.com/rust-embedded/wg/blob/master/rfcs/0523-msrv-2020.md

That is to say, no particular reason to choose 1.70 or 1.72, i just picked the latest

@thejpster
Copy link

What is the lowest Rust version that actually work for this crate? i.e. what actually is our minimum supported Rust Version at this time?

@Emilgardis
Copy link
Member Author

clap is the limiter pretty sure, so 1.70

@therealprof
Copy link
Contributor

I think we shouldn't be too agressive with moving the MSRV. It's certainly worthwile to be a bit conservative here. Also if we already know that the bump of clap requires 1.70 we shouldn't be mentioning that it might build with 1.64.

Unfortunately I've made the experience that between clap 2 and clap 3 there had been a ton of oh so subtle changes which broke some uses of my tools and took quite some time to discover and fix since the authors decided that it was undocumented behaviour and thus fair to silently change -- I'm a bit worried that we might run into similar issues. At the very least we would have a big fat warning in a few places to alert people to the possibility of unintended CLI breakage due to the clap bump.

@Emilgardis
Copy link
Member Author

Emilgardis commented Oct 8, 2023

we could also recommend installation with --locked, also #138 would mitigate this a bit since we prepare the binary

@Emilgardis Emilgardis requested a review from a team November 20, 2023 10:51
burrbull
burrbull previously approved these changes Nov 20, 2023
@burrbull
Copy link
Member

bors r+

@burrbull
Copy link
Member

need to migrate on merge queue

@burrbull burrbull added this pull request to the merge queue Nov 20, 2023
Merged via the queue into master with commit 19509f4 Nov 20, 2023
8 checks passed
@Emilgardis Emilgardis deleted the modernize branch November 20, 2023 16:03
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.

5 participants