Skip to content

Version optional#1044

Merged
Vlix merged 6 commits intoyesodweb:masterfrom
bellroy:version-optional
Sep 13, 2025
Merged

Version optional#1044
Vlix merged 6 commits intoyesodweb:masterfrom
bellroy:version-optional

Conversation

@lrworth
Copy link
Contributor

@lrworth lrworth commented Aug 30, 2025

Presented as a backward-compatible alternative to #1042.

Use of Paths_warp causes a large dependency closure when building with nix. See NixOS/nixpkgs#264434 for example. Building our application against this branch with the new flag disabled drastically reduces the closure size of our application by virtue of not including GHC and GCC as runtime dependencies.

I have made some minor refactors as requested by @Vlix in #1042.


Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

Closes #1042.

…sion

Disabling this flag prevents warp from depending on files in the GHC
distribution and may lead to a smaller runtime dependency closure.
Copy link
Contributor

@Vlix Vlix left a comment

Choose a reason for hiding this comment

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

I think this is acceptable. I can also imagine some people would not want to broadcast the warp version for security reasons, so this would kill two birds with one stone 👍 .

If all the CI runs correctly and you add the default to the flag, this can be merged :)

sendResponse,
sanitizeHeaderValue, -- for testing
-- Provided here for backwards compatibility.
warpVersion,

Choose a reason for hiding this comment

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

Question for the maintainers: should we deprecate this and remove it in a future version?

{-# DEPRECATED warpVersion "some message" #-}
warpVersion = Network.Wai.Handler.Warp.Settings.warpVersion

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel like it's worth it, but thanks for the suggestion 👍

@Vlix
Copy link
Contributor

Vlix commented Sep 9, 2025

I'll try to merge and publish this change this weekend.

@Vlix Vlix merged commit 947fc81 into yesodweb:master Sep 13, 2025
18 checks passed
@Vlix
Copy link
Contributor

Vlix commented Sep 13, 2025

Published as warp-3.4.9

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.

4 participants