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

Update CI script #26

Merged
merged 11 commits into from
Feb 22, 2024
Merged

Update CI script #26

merged 11 commits into from
Feb 22, 2024

Conversation

FreezyLemon
Copy link
Contributor

checkout v3 gives some warnings about outdated nodejs.

For dtolnay/rust-toolchain, it's not necessary to specify the toolchain variable when you use dtolnay/rust-toolchain@stable, @nightly etc.

@FreezyLemon
Copy link
Contributor Author

I missed that the codecov action also needs to be updated. I'll push a commit when I checked the potential breakage

@FreezyLemon
Copy link
Contributor Author

Looks like we can't update codecov/codecov-action yet:

https://github.com/codecov/codecov-action#breaking-changes:

Breaking Changes

Tokenless uploading is unsupported. However, PRs made from forks to the upstream public repos will support tokenless (e.g. contributors to OS projects do not need the upstream repo's Codecov token)

Someone would need to setup a token in the Codecov Web UI and set that as a secret for the GH action to use. Maybe in another PR.

That means this PR is done

@Luni-4
Copy link
Member

Luni-4 commented Feb 22, 2024

I have added a CODECOV_TOKEN for this repository. Perhaps we can remove the actions to install Rust and use the rustup commands directly, thus reducing the number of dependencies

@FreezyLemon
Copy link
Contributor Author

Sure, that should be relatively simple. The rust-toolchain action is nice for declaring versions like stable minus 5 releases or something which is cool for MSRV checking and such.

For simple stable/nightly installations, invoking it directly is also a good option. I'll change that in a minute

- Add token (now required)
- Fail CI if uploading fails (Errors would be silent otherwise)
Copy link

codecov bot commented Feb 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (24fce24) 66.52% compared to head (5ab548c) 66.52%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #26   +/-   ##
=======================================
  Coverage   66.52%   66.52%           
=======================================
  Files           4        4           
  Lines         923      923           
=======================================
  Hits          614      614           
  Misses        309      309           

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

@FreezyLemon
Copy link
Contributor Author

Removed the dependency on the toolchain action. This means we now rely on the fact that rustup is already installed, which should be a safe bet for github-hosted runners.

I also tentatively added the codecov-action update, let's hope it works (errors might only show up after merging, which is a bit of a pain)

@FreezyLemon
Copy link
Contributor Author

FreezyLemon commented Feb 22, 2024

This has turned into a bit of a mess.. I'm tempted to go back to the rust-toolchain action, it is way more convenient.

Either way, the PR should be squashed before merging.

@Luni-4
Copy link
Member

Luni-4 commented Feb 22, 2024

Ok for turning back to the rust-toolchain action. Can we move the GRCOV_VERSION at the start of the yml file as env variable, so we can control its version directly from there?

It is more convenient and less error-prone, plus it handles
some edge cases that might crop up in the future.
@FreezyLemon
Copy link
Contributor Author

Server error 500. I'll rerun it in a few minutes and hopefully see whether it's a real server error or something I changed.

@FreezyLemon
Copy link
Contributor Author

Looks like v4 of codecov/codecov-action is unreliable: codecov/codecov-action#1274

I'll roll back to v3 for now.

@FreezyLemon
Copy link
Contributor Author

Isn't CI fun?

I'll leave the codecov token stuff in for now, might as well get it out of the way.

@Luni-4
Copy link
Member

Luni-4 commented Feb 22, 2024

Try to remove fail_ci_if_error option and run v4, let's see what happens, I suspect it might be a spurious problem

@FreezyLemon
Copy link
Contributor Author

Sure. Not sure that's great either, but I'll try

@Luni-4
Copy link
Member

Luni-4 commented Feb 22, 2024

Yee, it worked!

@FreezyLemon
Copy link
Contributor Author

Of course it works now 🤦
I guess we could not fail the CI when the upload fails, but we should keep in mind that it does fail to upload sometimes. It seems to be a known problem, at least.

@FreezyLemon
Copy link
Contributor Author

Ah, actually, I think I should roll back the token thing for now. I'll open a PR from a branch on rust-av/v_frame to introduce those changes.

These should be done from a branch on the main repo (not fork)
to properly test the token
Copy link
Member

@Luni-4 Luni-4 left a comment

Choose a reason for hiding this comment

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

Yep, I agree! Approved in advance anyway!

@FreezyLemon FreezyLemon merged commit ecebaab into rust-av:main Feb 22, 2024
5 checks passed
@FreezyLemon FreezyLemon deleted the update-ci branch February 22, 2024 18:01
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.

2 participants