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

[20.09] haskellPackages.hakyll: fix build #97563

Closed

Conversation

erictapen
Copy link
Member

@erictapen erictapen commented Sep 9, 2020

Motivation for this change
Things done

I appended the commits from jaspervdj/hakyll#787.

Edit: This also fixes hakyll-contrib-hyphenation, so I marked it as unbroken.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@roberth
Copy link
Member

roberth commented Sep 9, 2020

The PR has not been merged, so the referenced commits may disappear when the PR is rebased.

@erictapen
Copy link
Member Author

Mh right, any idea how I could circumvent this?
There are already about 8 occurences of this pattern in the file, so I guess it is somewhat common?

Comment on lines +147 to +148
else overrideCabal super.hakyll {
patches = [
Copy link
Member

Choose a reason for hiding this comment

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

I think this only applies the patches if built on non-darwin? Is this logic correct?

patches = [
(pkgs.fetchpatch {
url = "https://github.com/jaspervdj/hakyll/pull/787/commits/b71955f7db1fd6532d3988be671281d1f6fcdd1d.patch";
sha256 = "Q9dFJpd9vmV1StjBpscDymprdF0Cdfil/tHD6c/ejBs=";
Copy link
Member

Choose a reason for hiding this comment

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

These sha256 hashes look strange? Are they base64 encoded or something?

I didn't even realize this was legal.

@@ -144,7 +144,22 @@ self: super: {
then dontCheck (overrideCabal super.hakyll (drv: {
testToolDepends = [];
}))
else super.hakyll;
else overrideCabal super.hakyll {
Copy link
Member

Choose a reason for hiding this comment

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

After fixing the above two problems, can you add a comment here that links to the PR containing these patches and explains why they are necessary?

@erictapen
Copy link
Member Author

Okay, let's stop this here.

I'm not willing to put any more energy into pushing such a minor change forward. The amount of bikeshedding I've encountered in the Haskell part of Nixpkgs is just too high for me as a low-volume contributor. Somehow this is really haskellPackages specific for me. For the last few months I consistently regretted it to have opened a PR to haskellPackages. That is not the case for any other part of Nixpkgs.

I'm not saying that the individual nitpicks in this PR and other PRs I opened aren't justified. It's just that I fail to see how meeting these high code standards stands in any relation to the utility of bringing these changes upstream. Most of the time when Hakyll broke in the last few years, there was no visible reaction in the NixOS community. This tells me, that only a few people use it. Why go through so many review iterations for this little gain?

I'm perfectly fine with just rebasing my changes ontop of my own branch, without bringing them upstream. Considering that bringing my changes upstream usually takes an order of magnitude more time than just fixing it for myself, it just doesn't add up to me anymore. I think I'll still open PRs to haskellPackages, but close them immediately, so other people in need of a fix can at least find and cherry-pick the solution.

@erictapen erictapen closed this Sep 10, 2020
@cdepillabout
Copy link
Member

cdepillabout commented Sep 10, 2020

@erictapen I assume you're talking about me, and I do appologize that you haven't had a good experience contributing to the Haskell part of Nixpkgs.

Let me try to explain some of my thinking so you can see where we're coming from.

The Haskell part of Nixpkgs has few regular contributors (only three of us). However, there is a relatively high volume of drive-by contributions. These are often one-off contributions by people that don't "follow-up" on stuff they get merged in. So if the stuff they submit becomes broken, it is up to the three regular maintainers to either fix it up or tear it out.

Without ample comments (for example above, why were certain patches applied to hakyll, and why were they only applied when building with Linux), it is pretty challenging for us to continue to maintain all the overrides in configuration-common.nix. This is doubly true when none of us personally use a package, or understand the reason for the overrides.

A lot of the work of fixing up Haskell stuff falls on @peti (since he does the weekly updates), so my main goal over the past year has been to make sure new contributions are well documented, and fit the normal style for the Haskell stuff. This lets the other maintainers (as well as new users) easily figure out the reason for the various overrides we apply here in nixpkgs.

I think I'll still open PRs to haskellPackages, but close them immediately, so other people in need of a fix can at least find and cherry-pick the solution.

Thanks for doing this! I think a lot of people search for old PRs before creating new ones, so this will hopefully save some people from figuring out everything from scratch.

@roberth
Copy link
Member

roberth commented Sep 10, 2020

Hakyll builds just fine in LTS-16.12, which Nixpkgs is based on. It seems to have been broken by 05bf532.

I would expect that updates on top of stackage are opt-in. Doing it that way increases the chance that stackage builds successfully, which has most of the important packages.

@erictapen
Copy link
Member Author

@cdepillabout Thank you for taking the time to elaborate on your perspective. First I'd like to make clear, that my rant wasn't about your reviews specifically, but more about a broader "the way thinks work" I noticed for quite some time in haskellPackages. Second I now understand better, that the high ratio of drive-by contributions vs. long-term maintainers is a situation with high incentives to avoid maintainenance debt. Maybe there really is no way around the high standards currently.

I think I'll take a break to bringing my commits through review, until I have more time/energy to do it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants