Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix Windows compilation by enabling kj-brotli #743

Merged
merged 1 commit into from
Jun 7, 2023
Merged

Conversation

fhanau
Copy link
Collaborator

@fhanau fhanau commented Jun 6, 2023

kj-brotli was only included in the build for unix platforms, which caused the Windows runner to fail. Not sure how this slipped through CI previously, all tests passed on the PR for this.

Edit: Looks like the Build & Release job is what's failing, that's why it wasn't caught earlier

@fhanau
Copy link
Collaborator Author

fhanau commented Jun 7, 2023

Actually this looks pretty difficult to fix. The brotli bazel build uses --pedantic-errors, a flag that does not exist in Windows clang-cl, and bazel does not seem to offer a way to change this. We may need a patch for brotli's build system, build it by invoking the Makefile or define our own build for it 😞

Edit: Created a modified copy of the brotli build file, that fixes it.

@fhanau fhanau force-pushed the felix/windows-br-fix branch 3 times, most recently from 10f7d61 to 89d5980 Compare June 7, 2023 13:27
@fhanau fhanau requested review from mikea, ohodson and mrbbot June 7, 2023 13:45
@fhanau fhanau force-pushed the felix/windows-br-fix branch from 89d5980 to 249f0e2 Compare June 7, 2023 16:09
Copy link
Contributor

@ohodson ohodson left a comment

Choose a reason for hiding this comment

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

This is honestly a bit beyond my bazel ability, but if it unblocks Windows compilation it's definitely a step in the right direction.

@fhanau fhanau merged commit f2aa142 into main Jun 7, 2023
@fhanau fhanau deleted the felix/windows-br-fix branch June 11, 2023 20:03
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants