-
Notifications
You must be signed in to change notification settings - Fork 30
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
Test no_std
in CI
#239
Test no_std
in CI
#239
Conversation
146c1ac
to
211d7c6
Compare
so, |
We don't want these on examples and such.
211d7c6
to
e899be5
Compare
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'm definitely happy to merge with a new variable added. Happy to alternatively see what Kaur thinks
@@ -111,10 +111,8 @@ jobs: | |||
with: | |||
save-if: ${{ github.event_name != 'merge_group' }} | |||
|
|||
# TODO: Add --target x86_64-unknown-none to the no_std check once we solve the compilation issues with it. | |||
# https://github.com/linebender/parley/issues/86 |
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.
What's the status of #86 once this PR is merged? I don't think this is currently marked as fixing that issue.
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.
Needs to be reviewed!
- name: cargo clippy (no_std) | ||
run: cargo hack clippy --workspace --locked --optional-deps --each-feature --ignore-unknown-features --features libm --exclude-features ${{ env.FEATURES_DEPENDING_ON_STD }} -- -D warnings | ||
run: cargo hack clippy ${{ env.RUST_MIN_VER_PKGS }} --locked --optional-deps --each-feature --ignore-unknown-features --features libm --exclude-features ${{ env.FEATURES_DEPENDING_ON_STD }} --target x86_64-unknown-none -- -D warnings |
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.
Hmm. I think the idiomatic approach here would be to add a new environment variable? cc @xStrom
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.
An even better solution is a brand new cargo-hack
feature --must-have-and-exclude-feature std
. Unfortunately it's been stuck in review queue for a month at cargo-hack#262.
For now I think it's fine to abuse ${{ env.RUST_MIN_VER_PKGS }}
and just get this landed. When the proper solution becomes possible we'll adjust it.
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.
Yes this contains a hack in the form of abusing ${{ env.RUST_MIN_VER_PKGS }}
, but the new state is still much better than the old one so let's land this.
Until I got distracted by work, I was going to suggest that |
One issue is that we could introduce published packages that don't support So my recommendation is to still just merge this as-is to get things moving, and the eventual standard change would be the more robust |
This builds on #238 and contains the same commit.
This has a change to only run
no_std
checks on the libraries since examples and such aren't likely to beno_std
, nor do they need to be.