-
Notifications
You must be signed in to change notification settings - Fork 12
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
Upgrade to the latest Linebender CI standard. #28
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -1,16 +1,15 @@ | |||
[package] | |||
name = "peniko" | |||
version = "0.1.0" | |||
license = "MIT OR Apache-2.0" | |||
license = "Apache-2.0 OR MIT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been meaning to ask why we keep making this change.
The Rust API guidelines suggest the other way around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Primarily for consistency, as that is the order we have it in file headers. For parsers it doesn't matter.
The funny thing with those Rust guidelines is that while yes they have MIT first in the Cargo.toml
property, just a few lines below that they describe how to write it in README.md
and there suddently Apache is first.
// doc_markdown was updated in https://github.com/rust-lang/rust-clippy/pull/11735 | ||
// to be more accurate, but our MSRV is earlier than that | ||
#![cfg_attr(not(peniko_msrv), warn(clippy::doc_markdown))] | ||
#![warn(clippy::doc_markdown)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this use the lints
table?
Hmm, I guess that's an MSRV hazard. Maybe it's fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah [lints]
would bump MSRV. Not worth it right now, but we should definitely adopt [lints]
later when MSRV allows.
@@ -12,9 +12,7 @@ | |||
//! [`kurbo`]: https://crates.io/crates/kurbo | |||
|
|||
#![cfg_attr(all(not(feature = "std"), not(test)), no_std)] | |||
// doc_markdown was updated in https://github.com/rust-lang/rust-clippy/pull/11735 | |||
// to be more accurate, but our MSRV is earlier than that | |||
#![cfg_attr(not(peniko_msrv), warn(clippy::doc_markdown))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 not running clippy on MSRV is a much better solution
This PR brings the latest Linebender CI script. It is mostly a copy-paste of the CI from Vello (as that is the most up-to-date right now) but there are some modifications.
Specifically we have the
std
/libm
situation here with Peniko (and also with Kurbo). So a simple--no-default-features
won't work.As was mentioned in #26,
cargo hack
does support--at-least-one-of std,libm
which works as advertised, but it's not really what we want because some features might requirestd
. Thus the solution is to have most checks run with--feature std
added and to also introduce an additionalno_std
check group with--features libm --exclude-features ${{ env.FEATURES_DEPENDING_ON_STD }} --target x86_64-unknown-none
. That way we can have a single configuration option as the list of std-dependant features to skip. Thex86_64-unknown-none
target ensures that we catch situations where our dependency tree accidentally tries to use std as this target doesn't supportstd
.I also committed
Cargo.lock
and upgraded dependencies inCargo.toml
to match the latest Linebender standard.