-
-
Notifications
You must be signed in to change notification settings - Fork 88
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.
Can you improve the description of the PR and also the commit messages? I don't know what the point of these changes is :)
What I don't like about this change is that we end up with more than 60 checks without having all the -sys enabled nor a windows/macos tests. I think that if we were to split the CI into a "matrix" for the configurations, it has to be per crate and it would run everything needed: build, test, clippy, rustfmt & gtk-rs checker |
.github/workflows/CI.yml
Outdated
@@ -17,6 +18,18 @@ jobs: | |||
- beta | |||
- nightly | |||
- "1.48.0" | |||
triple: |
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 a triple anymore :)
I would use something more generic as "projects" or "crates"
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.
First, making it work, then cleanup! 😆 But yes, you're absolutely right.
- { name: "graphene", features: "v1_10", nightly: "", test_sys: false } | ||
- { name: "gtk", features: "v3_24_9", nightly: "--all-features", test_sys: true } | ||
- { name: "pango", features: "v1_46", nightly: "--all-features", test_sys: false } | ||
- { name: "pangocairo", features: "", nightly: "--all-features", test_sys: true } |
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.
having features as the empty string failed for me because cargo does not like --features with no param... I went with having "args" (including the --features
part) in the table, this also lets you handle the examples
.github/workflows/CI.yml
Outdated
- name: "atk: tests nightly" | ||
run: xvfb-run --auto-servernum cargo test --manifest-path atk/Cargo.toml --all-features | ||
- name: tests nightly | ||
run: xvfb-run --auto-servernum cargo test --manifest-path ${{ matrix.triple.name }}/Cargo.toml ${{ matrix.triple.nightly }} |
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.
Can we run xvfb once as a service
instead of wrapping each command?
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 think there are actions that does that for you and make sure to shut down the service once the command has stopped running
So CI passed finally. How would you update it now @bilelmoussaoui ? |
.github/workflows/CI.yml
Outdated
# rustdoc-stripper | ||
- name: doc add | ||
run: cargo build --manifest-path ${{ matrix.conf.name }}/Cargo.toml --features "dox,embed-lgpl-docs" | ||
if: matrix.rust == 'nightly' && matrix.conf.name != 'glib' | ||
- name: doc removal | ||
run: cargo build --manifest-path ${{ matrix.conf.name }}/Cargo.toml --features "purge-lgpl-docs" | ||
if: matrix.rust == 'nightly' && matrix.conf.name != 'glib' | ||
- name: glib doc check | ||
run: cargo build --manifest-path ${{ matrix.conf.name }}/Cargo.toml --features "dox" | ||
if: matrix.rust == 'nightly' && matrix.conf.name == 'glib' | ||
- name: check diff | ||
run: git diff -R --exit-code |
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 think this one could be moved the to other job along with gtk-rs checker?
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.
Good idea.
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.
The clippy job should also use a matrix
@@ -17,6 +18,18 @@ jobs: | |||
- beta | |||
- nightly | |||
- "1.48.0" | |||
conf: |
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.
The checker job calls this crate
... I am fine with either, but I would be consistent (I will use the same for windows)
|
||
- uses: bcomnes/cleanup-xvfb@v1 | ||
|
||
build-others: |
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.
Why do we need to split build-others
to its own job? with a couple of tweaks it can be part of the main build
It finally worked! \o/ |
Gonna merge it as is and we'll use this as new base for further developments. |
No description provided.