Skip to content

Conversation

@photovoltex
Copy link
Member

Simple adjustment to the ci that checks all tls features and each-feature with native-tls.

We previously had --each-feature executions, but if --include-features is provided, each-feature will only execute for the features provided by that flag. Because of that we only tested rustls-tls-native-roots and native-tls.

This should hopefully prevent compilation issues like:

- check any tls feature
- check each-feature with native-tls
Copilot AI review requested due to automatic review settings November 11, 2025 08:43
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR simplifies the CI workflow by consolidating feature checking for the binary package. Instead of testing each TLS variant separately, it now tests all TLS options together and adds a specific check for native-tls with each-feature enabled.

Key changes:

  • Removed separate checks for individual TLS features (native-tls, rustls-tls-native-roots)
  • Removed separate checks for packages without TLS requirements (librespot-protocol)
  • Added consolidated feature checking that tests all three TLS options together
  • Added each-feature check specifically for native-tls

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@photovoltex
Copy link
Member Author

So... it compiles now successfully, but windows takes ~4 times longer to finish the run (lin: 2-3min, win: 9-10min). Currently that will apply for each PR and push. Any thoughts about how to reduce it or if that would be fine as-is?

Maybe running the windows flow only on dev as all features can be tested on linux?

@roderickvd
Copy link
Member

For me it’d be OK to just test each and all feature combinations on Linux and spare some cycles, unless we can think of more of a reason why we want to cover it on Windows just as defensive measure.

@photovoltex
Copy link
Member Author

So, adjusted it for now to only execute windows on the default branch. But I would like to have the option to trigger a windows run for a PR on demand if we go this route.

Manuel triggering a workflow seems to not allow it for a PR or external branch.

Maybe one of you can come up with an idea. Here are the available options for triggering.

What I had thought about so far as: pull_request_review: submitted, or pull_request: labeled seem interesting. Probably labeling a PR and by that deciding which OS to additionally run is probably the best and most intuitive I think? Let me know your thoughts on the topic^^

@kingosticks
Copy link
Contributor

Manuel triggering a workflow seems to not allow it for a PR or external branch.

I might misunderstand, but you can define a "base" workflow and then include it other workflows, which themselves can be manual or PR triggered. Does that help?

@photovoltex
Copy link
Member Author

Hmm, I might have formulated it badly then. I mean triggering a workflow for an existing PR with additional options, like adding another os to the CI execution. So that you can still test the execution of windows on-demand. For example, this PR would be a good example to also test windows, due to the changes on the windows ci.

But I think using labels to add another os to the PR CI execution might be a good solution.

@kingosticks
Copy link
Contributor

Yes, using labels sounds neat. Unless there's a obvious situation where you might add the label for book-keeping reasons but really not want the CI. But we don't really use labels otherwise so I don't think that's a real problem.

@photovoltex photovoltex changed the title CI: Improve feature checking for binary Draft: CI: Improve feature checking for binary Nov 20, 2025
@photovoltex photovoltex changed the title Draft: CI: Improve feature checking for binary CI: Improve feature checking for binary Nov 20, 2025
@photovoltex
Copy link
Member Author

Hmm, I had it working temporarily, but seem to stopped working... you have to love ci

@photovoltex
Copy link
Member Author

So, I just made a wrong assumption what the check for "is a PR" was. I also improved how the label is named, so that the label is CI: Windows and something upcoming for macos could be CI: MacOS.

@photovoltex photovoltex added the CI: Windows Adds windows to the CI execution label Nov 20, 2025
@photovoltex
Copy link
Member Author

So, I'm done here. Works as I want it to do. I prepared something for macOS already, but will put this in a follow-up PR. Anything that still needs to be addressed or any concerns in regards to the changes? Otherwise, I would merge it in the coming days :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: Windows Adds windows to the CI execution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants