Skip to content
This repository has been archived by the owner on Sep 21, 2024. It is now read-only.

Bolster CI Actions #96

Closed

Conversation

igbanam
Copy link
Contributor

@igbanam igbanam commented Oct 19, 2022

This change brings some extra functionality into the CI process. Using Github Actions, we should be able to

  • Run lint checks on change-sets
  • Audit dependencies
  • Runs the formatting tool

…on pull requests.

p.s: I've kept this away from pushes on the invariant that the development process here receives all contributions to the main branch through some pull request of some sort.

Fixes #94

@cdata
Copy link
Collaborator

cdata commented Oct 19, 2022

@igbanam I'm not totally sure if this is the issue, but I think you need to set the working directory for some of these steps because our code in this project is one level deep in a rust sub-directory. Example from our other workflow:

working-directory: ./rust

@igbanam
Copy link
Contributor Author

igbanam commented Oct 20, 2022

@cdata I made the updates to the relevant checks. Checks are green now.

@cdata
Copy link
Collaborator

cdata commented Oct 20, 2022

@igbanam I'm not seeing the check for the new workflow you added - I think it might have a typo in it somewhere (just referring to the message here: https://github.com/subconsciousnetwork/noosphere/actions/runs/3285863349). Are you seeing the workflow run for you?

@igbanam
Copy link
Contributor Author

igbanam commented Oct 20, 2022

I just realized it's failing early for me too.

Root Cause: The working-directory key only works for runs. actions-rs/cargo currently does not support changing working directory — but that issue is laden with forks with potential solutions. I'd have to go through these forks and select one which works for this use case. Worst case scenario, I'll add it myself.

@igbanam
Copy link
Contributor Author

igbanam commented Oct 21, 2022

Oh… we have lift-off 🚀

The linters are catching errors now.

I'll fix the linter errors as part of this PR

@igbanam
Copy link
Contributor Author

igbanam commented Oct 22, 2022

I'm gonna pause here for your input, @cdata.

I trust the Rust tooling and all… but this is making more changes to your code than I am comfortable with. I'm checking that existing tests don't break per change; and we are green so far. But please have a look through and let me know if you're comfortable with my going on with this.

p.s: take a look at the current checker failures as well to know the nature of possible future changes.

@igbanam
Copy link
Contributor Author

igbanam commented Oct 23, 2022

Chose not to deny warnings because my rust-fu couldn't handle the refactor being asked.

╭─   ~/p/i/noosphere/rust on   chore/bolster-ci-actions ⇡2
╰─❯ cargo +stable clippy --all -- -D warnings                                                     ─╯
    Checking noosphere-cbor v0.1.0 (/Users/igbanam/projects/igbanam/noosphere/rust/noosphere-cbor)
    Checking noosphere-storage v0.1.0 (/Users/igbanam/projects/igbanam/noosphere/rust/noosphere-storage)
    Checking noosphere-collections v0.1.0 (/Users/igbanam/projects/igbanam/noosphere/rust/noosphere-collections)
error: module has the same name as its containing module
 --> noosphere-collections/src/hamt/mod.rs:7:1
  |
7 | mod hamt;
  | ^^^^^^^^^
  |
  = note: `-D clippy::module-inception` implied by `-D warnings`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#module_inception

error: very complex type used. Consider factoring parts into `type` definitions
   --> noosphere-collections/src/hamt/hamt.rs:371:36
    |
371 |     pub fn stream<'a>(&'a self) -> Pin<Box<dyn Stream<Item = Result<(&'a K, &'a V)>> + 'a>> {
    |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::type-complexity` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity

error: very complex type used. Consider factoring parts into `type` definitions
   --> noosphere-collections/src/hamt/node.rs:156:10
    |
156 |     ) -> Pin<Box<dyn Stream<Item = Result<(&'a K, &'a V)>> + 'a>>
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity

error: could not compile `noosphere-collections` due to 3 previous errors

I tried refactoring the complext type with what I read up on the interwebs, but I got met with rust-lang/rust#8995 which — after reading through — seems it's almost addressed, but not yet.

@igbanam
Copy link
Contributor Author

igbanam commented Oct 23, 2022

If we truly want to keep the high Rust bar, we can tune this up later.

@cdata
Copy link
Collaborator

cdata commented Oct 25, 2022

@igbanam as part of #107 we ran cargo clippy --fix and cleared up all the warnings. I would recommend rebasing your change without the warning fixes, just the new workflow, and see how it goes!

p.s: I'm using Github to build and test because Ventura — as with all OS
X updates — comes with its set of woes.
@cdata
Copy link
Collaborator

cdata commented Feb 20, 2023

Thanks for putting in the work on this change. Since it's getting stale, I'm going to close out the PR for now, but if you feel inspired to pick it up again we will re-open it right away.

@cdata cdata closed this Feb 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update CI workflow to crib some patterns from rs-ucan
2 participants