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.

Sign up for GitHub

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

Jump to bottom

ci/Check dependencies #895

Merged
merged 17 commits into from
Nov 19, 2023
Merged

ci/Check dependencies #895

merged 17 commits into from
Nov 19, 2023

Conversation

Luni-4
Copy link
Collaborator

@Luni-4 Luni-4 commented Oct 24, 2023

Pull Request Template

Checklist

  • Confirm that run-checks script has been executed.

Related Issues/PRs

Provide links to relevant issues and dependent PRs.

Changes

This PR adds dependencies checks to ci in order to detect:

  • Vulnerabilities
  • Duplications
  • Unused

Testing

Describe how these changes have been tested.

@Luni-4 Luni-4 force-pushed the dependencies branch 2 times, most recently from aad67c1 to 71fa4d4 Compare October 24, 2023 16:30
@codecov
Copy link

codecov bot commented Oct 24, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (945014b) 87.50% compared to head (8605db6) 87.41%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #895      +/-   ##
==========================================
- Coverage   87.50%   87.41%   -0.10%     
==========================================
  Files         502      502              
  Lines       51080    50966     -114     
==========================================
- Hits        44697    44550     -147     
- Misses       6383     6416      +33

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Luni-4
Copy link
Collaborator Author

Luni-4 commented Oct 24, 2023

@nathanielsimard

This PR is ready to be reviewed

@nathanielsimard
Copy link
Member

@Luni-4 We should fix the CI dependencies step before merging!

@Luni-4
Copy link
Collaborator Author

Luni-4 commented Oct 24, 2023 •
edited
Loading

@nathanielsimard

Feel free to change my PR in order to fix the burn dependencies :)

@Luni-4 Luni-4 force-pushed the dependencies branch 4 times, most recently from e8e2b47 to 7ede25a Compare October 25, 2023 13:26
@Luni-4
Copy link
Collaborator Author

Luni-4 commented Oct 25, 2023

@nathanielsimard

It is ready to be reviewed

@Luni-4
Copy link
Collaborator Author

Luni-4 commented Oct 26, 2023

@nathanielsimard @antimora

I have added a files to check licenses but I do not know if I had configured that correctly. Review cargo.deny options carefully and see if they are right the ones I set up

@antimora
Copy link
Collaborator

It seems the dependencies is doing its job and failing the CI. We should review the list of licenses and add to an allowable list. There seems to be some odd licenses that I am not familiar with so it might take some time to review.

@Luni-4
Copy link
Collaborator Author

Luni-4 commented Nov 13, 2023

@antimora

Since I do not have the skills to evaluate the right licenses for this project, I'm leaving this PR to you. Feel free to update it and change the code whatever you want, thanks a lot in advance!

@Luni-4
Copy link
Collaborator Author

Luni-4 commented Nov 16, 2023

@nathanielsimard and @antimora

After a brief dialogue with @antimora, I was able to set up and fix the dependency checker. Below a list of the accomplished things:

  • Multiple dependencies are detected but they are not going to stop the CI. I have set up a warn level instead of an error level

  • The following licenses are deemed acceptable:

    Apache-2.0 WITH LLVM-exception
    Apache-2.0
    BSD-3-Clause
    CC0-1.0
    ISC
    MIT
    MPL-2.0
    OpenSSL
    Unicode-DFS-2016
    Unlicense
    Zlib
  • Every unused dependency has been removed

nathanielsimard
nathanielsimard approved these changes Nov 17, 2023
View reviewed changes
Copy link
Member

@nathanielsimard nathanielsimard left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for @antimora to review as well before merging

.github/workflows/dependencies.yml
Comment on lines +48 to +50
curl -L "$UDEPS_LINK/$UDEPS_VERSION/cargo-udeps-$UDEPS_VERSION-x86_64-unknown-linux-gnu.tar.gz" |
tar xz -C $HOME/.cargo/bin --strip-components 2

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to use the following: cargo install cargo-udeps --locked ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To install faster the binary, reducing the CI time.

antimora
antimora approved these changes Nov 17, 2023
View reviewed changes
Copy link
Collaborator

@antimora antimora left a comment

Choose a reason for hiding this comment

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

Looks good. Lets a give a try!

Thanks, @Luni-4 for your work with this CI enhancements. I feel we have a world class tooling now =)

@nathanielsimard nathanielsimard merged commit 4456034 into tracel-ai:main Nov 19, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanielsimard nathanielsimard nathanielsimard approved these changes

@antimora antimora antimora approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Luni-4 @nathanielsimard @antimora