Skip to content

Add ci check for duplicate dependencies#1114

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
tcharding:07-21-cargo-tree-d
Jul 22, 2022
Merged

Add ci check for duplicate dependencies#1114
apoelstra merged 1 commit intorust-bitcoin:masterfrom
tcharding:07-21-cargo-tree-d

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Jul 21, 2022

Add a call to cargo tree --duplicates in the ci script to ensure that we do not have any duplicated dependencies.

Kudos to Kixunil for the idea (over in: #1104)

Kixunil
Kixunil previously approved these changes Jul 21, 2022
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 521b119720d66d1298a0228b1a514578c8079af7

I don't mind re-reviewing if you quote the variable. :)

contrib/test.sh Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Bash paranoia mindset says you should put double quotes around variables (unless intentional). In this case if wc screwed up and printed a space. (Unlikely but a good habit.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure thing, can do. Cheers.

@dpc
Copy link
Copy Markdown
Contributor

dpc commented Jul 21, 2022

I don't know how much does this check add to the actions time, but possibly it could be a part of a different workflow - eg. one running every 24 hours. It's not exactly critical or urgent to fix these.

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jul 21, 2022

@dpc on my machine with bunch of browser tabs spinning CPU cargo tree -d took ~2 seconds, so not worth the effort. :)

Add a call to `cargo tree --duplicates` in the ci script to ensure that
we do not have any duplicated dependencies.

Kudos to Martin for the idea.
@tcharding
Copy link
Copy Markdown
Member Author

Changes in force push: Add quotes around variable as suggested, no other changes.

@dpc
Copy link
Copy Markdown
Contributor

dpc commented Jul 21, 2022

BTW for shellscript, I recommend to always use a shellcheck

Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK cda097d

@tcharding
Copy link
Copy Markdown
Member Author

BTW for shellscript, I recommend to always use a shellcheck

BOOM! shellcheck found a bunch of bugs!

@tcharding
Copy link
Copy Markdown
Member Author

I ran shellcheck on #1111 as well and all but one of the bugs had been fixed, so I pushed one additional patch onto that branch. shellcheck for the win.

Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK cda097d

@apoelstra apoelstra merged commit c7b8d4c into rust-bitcoin:master Jul 22, 2022
@tcharding tcharding deleted the 07-21-cargo-tree-d branch July 25, 2022 03:29
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
…ndencies

cda097d Add ci check for duplicate dependencies (Tobin C. Harding)

Pull request description:

  Add a call to `cargo tree --duplicates` in the ci script to ensure that we do not have any duplicated dependencies.

  Kudos to Kixunil  for the idea (over in: rust-bitcoin/rust-bitcoin#1104)

ACKs for top commit:
  apoelstra:
    ACK cda097d
  Kixunil:
    ACK cda097d

Tree-SHA512: 77f07dd5c6794b5a59293bd62bda0fe61384a30cf8258e79aca9ce32090f869f0a13929b6a7a4c35e10fc653968b12ddd4c291df9ecd0962632017f59c81d025
@Kixunil Kixunil added this to the 0.29.0 milestone Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants