search

dbrgn / tealdeer

A very fast implementation of tldr in Rust.
https://dbrgn.github.io/tealdeer/
Apache License 2.0
4.06k stars 123 forks source link

Update CI workflows #324

Closed kbdharun closed 1 year ago

kbdharun commented 1 year ago

Changes

niklasmohrin commented 1 year ago

Hi :) These changes seem nice, but they make it a bit intransparent as to what versions are used. For example, I dont see which version of the rust compiler is running (and we also want to build with two specific versions: the msrv and stable). About the OS update: not sure either, but should probably be fine

kbdharun commented 1 year ago

Hi :) These changes seem nice, but they make it a bit intransparent as to what versions are used. For example, I don't see which version of the rust compiler is running (and we also want to build with two specific versions: the msrv and stable).

Sorry for the delay, I wasn't feeling well. I have updated the workflow now to support multiple versions of Rust, a test action run can be found here [Note: the failed actions are because I am running it in a fork, it should run fine here, also by the way can you approve the workflow run]

About the OS update: not sure either, but it should probably be fine

Yep, it is better to use the latest stable versions to have faster CIs and additional features. Let me know If you guys need me to revert it.

niklasmohrin commented 1 year ago

Hi, I think it is still not correct: You have added the line back in that starts more CI runs with different values for the rust variable, but this variable is never used (note the use of ${{ matrix.rust }} before when installing the toolchain). Currently, it is still the preinstalled version of Rust that is used, which is not what we want for the reasons I wrote before.

I was unaware that actions-rs is not maintained anymore, so switching to something else seems like a good idea. From what I can tell from https://github.com/actions-rs/toolchain/issues/216 and for example https://github.com/jonhoo/rust-ci-conf/commit/362696ab8007ef1a4779885a398286856cacf555 the world is moving to dtolnay's action, so I think going with that would be a sane move :)

Note: the failed actions are because I am running it in a fork

Actually, I think that you forgot to add the checkout action back in in the two jobs that are failing as both fail because of a missing Cargo.toml file :)

About using latest os, I was wondering if we would maybe want to stay with the older versions so that we don't miss an incompatibility with for example windows 10, ubuntu 22.04 or so. @dbrgn what do you think about this?

kbdharun commented 1 year ago

Hi, I think it is still not correct: You have added the line back in that starts more CI runs with different values for the rust variable, but this variable is never used (note the use of ${{ matrix.rust }} before when installing the toolchain). Currently, it is still the preinstalled version of Rust that is used, which is not what we want for the reasons I wrote before.

Noted

I was unaware that actions-rs is not maintained anymore, so switching to something else seems like a good idea. From what I can tell from actions-rs/toolchain#216 and for example jonhoo/rust-ci-conf@362696a the world is moving to dtolnay's action, so I think going with that would be a sane move :)

Agreed, I will try to update this PR to https://github.com/dtolnay/rust-toolchain as even with matrix jobs it is possible for some old versions of packages to be dropped from runner images i.e https://github.com/actions/runner-images/blob/main/images/linux/Ubuntu2204-Readme.md.

Note: the failed actions are because I am running it in a fork

Actually, I think that you forgot to add the checkout action back in in the two jobs that are failing as both fail because of a missing Cargo.toml file :)

Oops, just now noticed it will fix it.

About using latest os, I was wondering if we would maybe want to stay with the older versions so that we don't miss an incompatibility with for example windows 10, ubuntu 22.04 or so. @dbrgn what do you think about this?

Regarding this, the official GitHub runner doesn't support Windows 10 runners while the ubuntu-latest runner is the latest LTS Ubuntu version which is 22.04 now. Do note if you need I could add Ubuntu 20.04 to tests but the runner will get removed next year when 24.04 gets released as GitHub only keeps 2 LTS versions of Ubuntu runners at a time. An alternative is running the job in a container (as GitHub self-hosted runners aren't advisable for public projects)

Edit. Done updated the CI to the suggested toolchain action, and all jobs have succeeded https://github.com/kbdharun/tealdeer/actions/runs/5124671837 (as requested I added an older supported runner for Ubuntu 20.04)

kbdharun commented 1 year ago

Done, I think it is GTG now.

dbrgn commented 1 year ago

This looks good to me, let's see if CI runs through! If yes, I think we can merge this.

Thanks for your work and your patience, @kbdharun and @niklasmohrin 🙂