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

Make alertmanager client compile with ghc9 #3

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jappeace
Copy link

I made it reproducible with nix.
I allow for cabal builds with dedicated shells.
I extended the documentation in readme.
I fixed all ghc9 compile errors.

jappeace added 6 commits May 27, 2022 10:47
upgrade http-cleint

increase minimum aeson bound

increase version bounds
Add naive shells to allow cabal builds
@jappeace jappeace requested a review from ivb-supercede May 27, 2022 08:50
@jappeace jappeace changed the title Make it compile with ghc9 Make alertmanager client compile with ghc9 May 27, 2022
@@ -30,16 +30,16 @@ library
lib
ghc-options: -Wall -funbox-strict-fields
build-depends:
aeson >=1.0 && <2.0
aeson >=2.0 && <3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Annoying that this big version bump is necessary. I wish the OpenAPI people had supported multiple aeson versions, but it seems that the Aeson maintainers aren't supporting the 1.* releases anymore.

Copy link
Author

Choose a reason for hiding this comment

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

aeson 1.xx is susceptible to DoS attacks. It's possible to create a universal hash collision for FNV based hash algorithms.

haskellPackages = (if compiler == "default"
then pkgs.haskellPackages
else pkgs.haskell.packages.${compiler}).extend(self: super: {
alertmanager-openapi = haskellPackages.callPackage ../alertmanager-openapi {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is explicitly adding the package necessary here but not above?

Copy link
Author

Choose a reason for hiding this comment

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

it's not available in the package set. no-one uploaded it to hackage.
Even if it was you want to override it to the current version in the tree rather then the one on hackage because it may contain changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, my question wasn't clear - why isn't it necessary to add alertmanager-openapi to the package set when the compiler choice is "default"? It won't be in the Hackage set in either case.

Copy link
Author

Choose a reason for hiding this comment

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

otherwise cabal can't find it, nix patches cabal somehow

Comment on lines +7 to +20
f = { mkDerivation, alertmanager-openapi, base, http-client, lens
, lib, text, transformers, cabal-install
}:
mkDerivation {
pname = "alertmanager-client";
version = "0.1.0.0";
src = ./.;
libraryToolDepends = [ cabal-install ];
libraryHaskellDepends = [
alertmanager-openapi base http-client lens text transformers
];
homepage = "https://github.com/SupercedeTech/alertmanager-client#readme";
license = lib.licenses.bsd3;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use callCabal2Nix?

Copy link
Author

Choose a reason for hiding this comment

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

because many, /many/ more packages are broken, I considered going from me not being able to build it at all to having it generate and build the new bindings good enough for this upgrade cycle.

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.

2 participants