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.

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

Add windows build support #249

Merged
merged 7 commits into from Jun 1, 2023
Merged

Add windows build support #249

merged 7 commits into from Jun 1, 2023

Conversation

zzl221000
Copy link
Contributor

Add the vendored feature of sasl2-sys library

Copy link
Contributor

@whoahbot whoahbot left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to troubleshoot windows builds.

@@ -0,0 +1,31 @@
name: WINTEST
on:
workflow_dispatch:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this workflow is configured to run on the workflow_dispatch event, we won't be able to run it, as workflow_dispatch events can only be triggered if they are present in the main branch of the repository.

I think it would be a good idea to add this build to .github/workflows/CI.yml, so we can try building it and running the test suite for windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

workflow_dispatch is only for testing purposes, after all, windows build requires a special environment.
Another reason for providing the windows.yml file is that I can't build the sasl2-sys library locally, but github action can, so maybe I'm missing some necessary build dependencies.

I also think it is a good idea to merge that workflow into CI.

- build: windows-vendored
os: windows-latest
rust: stable
features: vendored,openssl-vendored
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these matrix features used? I don't see them referenced in this action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These matrix features are a direct use of rust-sasl's workflow, and perhaps we can do some optimization of this

https://github.com/MaterializeInc/rust-sasl/blob/master/.github/workflows/main.yml

[target.'cfg(target_os = "windows")'.dependencies]
sasl2-sys = {version="0.1.20",features=["vendored","openssl-vendored"]}

[build-dependencies]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also be conditionally set for windows builds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on these issues, it can be determined that the force-engine feature is required for windows and musl platforms
fede1024/rust-rdkafka#572 (comment)
fede1024/rust-rdkafka#446
alexcrichton/openssl-src-rs#151

Cargo.toml Show resolved Hide resolved
@whoahbot
Copy link
Contributor

whoahbot commented May 31, 2023

/cc #248

@zzl221000
Copy link
Contributor Author

@whoahbot
I've merged the windows workflow into ci.

@zzl221000
Copy link
Contributor Author

@whoahbot fixed test_cluster_can_be_ctrl_c in windows

@whoahbot whoahbot merged commit 4a301bc into bytewax:main Jun 1, 2023
26 checks passed
@whoahbot
Copy link
Contributor

whoahbot commented Jun 1, 2023

@zzl221000 Thanks for putting this PR together.

I need to change one more thing in the CI/CD workflow in order to publish this version to PyPI. I'll make that change in a subsequent PR when we create the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants