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

Add support GitHub Actions #166

Closed
wants to merge 8 commits into from
Closed

Add support GitHub Actions #166

wants to merge 8 commits into from

Conversation

Mephistophiles
Copy link
Contributor

@Mephistophiles Mephistophiles commented Jan 17, 2021 •
edited
Loading

This PR introduce:

  • fix clippy errors
  • added GitHub Actions support:
    • Every push check:
      • run cargo-test (compile test and launch test suits)
      • run cargo-clippy (check additional lints)
      • run cargo-audit (check for rust vulnerabilities)
      • run cargo-fmt (check code format)
    • Every PR check

The jobs use an ubuntu-latest image with an archlinux docker inside.

P.S. cargo clippy --all-features failed (alpm package failed)

TODO:

@Mephistophiles Mephistophiles marked this pull request as ready for review January 17, 2021 15:09
@Morganamilo
Copy link
Owner

The code changes are probably fine. The github actions stuff though I'll need some explanation of some of the bits.

Morganamilo
Morganamilo reviewed Jan 17, 2021
View reviewed changes
.github/workflows/rust.yml Outdated
pull_request:
push:
branches:
- master
Copy link
Owner

Choose a reason for hiding this comment

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

Id prefer this on every branch. I don't always work on master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll fix that.

.github/workflows/rust.yml Outdated

container:
image: archlinux:base-devel
options: --privileged
Copy link
Owner

Choose a reason for hiding this comment

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

What does this do and why is it needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed for using archlinux instead of ubuntu (default linux container)

Copy link
Owner

Choose a reason for hiding this comment

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

Works for me without it at https://github.com/Morganamilo/alpm.rs/blob/master/.github/workflows/alpm.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I apologize for not immediately thinking to do so :(

.github/workflows/rust.yml Outdated
with:
fetch-depth: 0

- name: Install Rust toolchain
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer to just use pacman here. Less complex and has the added benefit of also testing paru compiles with the version of rust in the repos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to rust from archlinux

.github/workflows/rust.yml Outdated
- /sys/fs/cgroup:/sys/fs/cgroup

steps:
- name: Install libclang (needed by clippy)
Copy link
Owner

Choose a reason for hiding this comment

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

clippy does not need clang afaik where did you get that from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

.github/workflows/rust.yml Outdated

steps:
- name: Install libclang (needed by clippy)
run: sudo pacman --noconfirm -Syy clang
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't -Sy or -Syy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cache has been removed from the archive images archive.

warning: database file for 'core' does not exist (use '-Sy' to download)
warning: database file for 'extra' does not exist (use '-Sy' to download)
warning: database file for 'community' does not exist (use '-Sy' to download)

Copy link
Owner

Choose a reason for hiding this comment

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

Then use -Syu to avoid a partial upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, After installing rust from the official repository, the clang was not required.

Copy link
Owner

Choose a reason for hiding this comment

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

Also forgot to mention, isn't the container running as root? the sudos shouldn't be needed right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes. Removed.

.github/workflows/rust.yml Outdated
- uses: actions-rs/clippy-check@v1
with:
token: ${{ secrets.GITHUB_TOKEN }}
args: #--all-features (TODO: fix all features build)
Copy link
Owner

Choose a reason for hiding this comment

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

Don't just blindly test all features. There needs to be a separate test for git and non git builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I'll do it a little later.

@Mephistophiles Mephistophiles force-pushed the actions branch 5 times, most recently from 579c111 to a2fe74a Compare January 17, 2021 16:34
@Mephistophiles
Copy link
Contributor Author

@Morganamilo small question: do you need the build badge in the README.md?
example: Rust

@Mephistophiles Mephistophiles force-pushed the actions branch 3 times, most recently from ce7b03a to 699faff Compare January 17, 2021 16:56
@Morganamilo
Copy link
Owner

Not really, the commit already shows the ci status.

@Mephistophiles
add github actions for lint input commits
12b3c00 
So far done:
* cargo audit (audir rust vulnerabilities)
* cargo test
* cargo clippy
* cargo fmt
@Mephistophiles Mephistophiles marked this pull request as draft January 17, 2021 17:00
Morganamilo
Morganamilo reviewed Jan 17, 2021
View reviewed changes
.github/workflows/rust.yml

- uses: actions-rs/clippy-check@v1
with:
token: ${{ secrets.GITHUB_TOKEN }}
Copy link
Owner

Choose a reason for hiding this comment

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

Also why is the secret key needed for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only needed for forked repos. I will remove it before change to "Ready for Review" state.

@Morganamilo
Copy link
Owner

By the way, if you're interested in doing CI at all, It would be nice to get #83 solved. Yay does it with this https://github.com/Jguer/yay/blob/next/.github/workflows/multiarch-build.yml but I'm not really familiar with github actions or docker.

@patrickelectric
Copy link

Hey, any update ?

@Morganamilo Morganamilo closed this May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Morganamilo Morganamilo Morganamilo left review comments

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
@Mephistophiles @Morganamilo @patrickelectric