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

feat(ci): feature checks #5789

Merged
merged 1 commit into from
Sep 10, 2023
Merged

feat(ci): feature checks #5789

merged 1 commit into from
Sep 10, 2023

Conversation

bernard-wagner
Copy link
Contributor

Motivation

See #5788

Ensure that cargo install works for all binaries as some workspace dependencies hide features that need to be explicitly declared by crates.

bernard-wagner
bernard-wagner commented Sep 6, 2023
View reviewed changes
.github/workflows/test.yml Outdated
target: [forge, cast, chisel, anvil]
name: cargo install
runs-on: ubuntu-latest
needs: test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't want to run this unless the rest of the CI passes.

@mattsse mattsse mentioned this pull request Sep 6, 2023
Closed
@DaniPopes
Copy link
Member

DaniPopes commented Sep 8, 2023 •
edited
Loading

This job can be replaced by just cargo hack check which runs cargo check on all crates. Installing from remote with optimizations is not necessary, and so is waiting for tests to pass as they're unrelated.

See also feature checks job in alloy-rs/core (note we don't need to check all features since we barely have any, so cargo hack check should be enough): https://github.com/alloy-rs/core/blob/4c81e0099333c0001da6f886e698634e26c36de3/.github/workflows/ci.yml#L43-L53

@DaniPopes DaniPopes mentioned this pull request Sep 8, 2023
Merged
@bernard-wagner bernard-wagner force-pushed the ci-check-install branch 2 times, most recently from 293d0a9 to aea9c78 Compare September 8, 2023 05:40
@bernard-wagner bernard-wagner changed the title feat(ci): check cargo install feat(ci): features check Sep 8, 2023
@bernard-wagner bernard-wagner changed the title feat(ci): features check feat(ci): feature checks Sep 8, 2023
Evalir
Evalir reviewed Sep 8, 2023
View reviewed changes
.github/workflows/test.yml
@@ -152,3 +152,15 @@ jobs:
- uses: Swatinem/rust-cache@v2
- name: forge fmt
run: cargo run --bin forge -- fmt --check testdata/

feature-checks:
Copy link
Member

Choose a reason for hiding this comment

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

wonder how much time this will add to ci

Copy link
Member

Choose a reason for hiding this comment

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

should complete before tests if it runs in parallel with them

@mattsse mattsse marked this pull request as ready for review September 10, 2023 09:35
@mattsse mattsse merged commit 3a53c9b into foundry-rs:master Sep 10, 2023
16 checks passed
mikelodder7 pushed a commit to LIT-Protocol/foundry that referenced this pull request Sep 12, 2023
@bernard-wagner @mikelodder7
feat(ci): feature-checks (foundry-rs#5789)
665089d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaniPopes DaniPopes DaniPopes left review comments

@Evalir Evalir Evalir left review comments

@mattsse mattsse mattsse approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bernard-wagner @DaniPopes @mattsse @Evalir