polysemy-plugin: fix tests #71967

sevanspowell · 2019-10-25T07:10:47Z

Motivation for this change

polysemy-plugin is broken due to failing tests.

Things done

polysemy-plugin was broken due to failing doctests:
polysemy-plugin: Make plugin available to doctest #71164.
I submitted a PR upstream to fix this:
Use cabal-doctest polysemy-research/polysemy#265.
I've applied the patch of the PR here and moved the default polysemy
attribute to "polysemy_1_2_0_0" because polysemy-plugin requires
"polysemy >= 1.2.0.0".
Move default "polysemy-zoo" attribute to "polysemy-zoo_0_6_0_1"
because it is fixed by the polysemy-plugin changes and fixes issues
with building "polysemy-RandomFu" and "knit-haskell".

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
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 nix-review --run "nix-review wip"
Ensured that relevant documentation is up to date
Fits CONTRIBUTING.md.

Notify maintainers

cc @peti

sevanspowell · 2019-10-25T07:10:59Z

Now I've done a couple of things that might be frowned upon, I would love some help with these:

Checked-in a patch file instead of using fetchpatch.
- The PR diff from github applies to the polysemy project as a whole, but the polysemy-plugin.src only contains the plugin part. As a result the diff is weird and I couldn't get the right fetchpatch setting to make it work.
Manually altered "hackage-packages.nix" to make sure "cabal-doctest" was in the "testToolDepends" of the derivation, is there another way to do this?

No longer broken:

polysemy-plugin
axel
polysemy-zoo_0_6_0_1
polysemy-RandomFu
knit-haskell

Still broken:

co-log-polysemy (polysemy >=0.2.1.0 && <0.3)
gamgee (polysemy >=1.0.0.0 && <1.1)

pkgs/development/haskell-modules/configuration-common.nix

cdepillabout · 2019-10-25T10:58:32Z

pkgs/development/haskell-modules/configuration-common.nix

+  # The polysemy-plugin tests failed because it couldn't find
+  # the polysemy-plugin package in the doctests:
+  # https://github.com/NixOS/nixpkgs/issues/71164
+  # I've addressed this with a PR upstream:
+  # https://github.com/polysemy-research/polysemy/pull/265
+  # The patch of which is applied here.
+  polysemy-plugin = appendPatch super.polysemy-plugin ./patches/polysemy-plugin.patch;


If possible, we don't like to carry around packages in the nixpkgs repo.

Also, it is generally easier for us to just disable the tests with dontCheck instead of trying to patch them.

Please change this to the dontCheck approach (or you could wait until polysemy-research/polysemy#265 is merged and a new release is made).

It is generally easier for us to just disable the tests with dontCheck instead of trying to patch them.

That is true, we hardly ever patch test suites simply because that tends to be more effort than just disabling them. There is one argument to favor of patching over disabling, though: When a new version of the package comes out that incorporates the fixes, our patch will fail to apply -- which means that someone notices that the corresponding override can be deleted. That is not true for dontCheck; those overrides won't fail when a new version comes out and therefore we usually don't remove them even though the override has become obsolete.

cdepillabout · 2019-10-25T11:03:18Z

pkgs/development/haskell-modules/hackage-packages.nix

@@ -185893,7 +185894,7 @@ self: {
         base containers doctest ghc ghc-tcplugins-extra hspec
         inspection-testing polysemy should-not-typecheck syb transformers
       ];
-       testToolDepends = [ hspec-discover ];
+       testToolDepends = [ hspec-discover cabal-doctest ];


You're correct that this file is autogenerated, so it shouldn't be edited by hand.

Above I suggest just using dontCheck for polysemy-plugin, but in case you're curious, you can grep through pkgs/development/haskell-modules/configuration-common.nix to see how people add test tools.

One solution is to use the addBuildTool function. The other solution is to use overrideCabal and explicitly set testToolDepends.

cabal-doctest should probably go into setupHaskellDepends since it's a part of the Setup.hs build. This means that this dependency will be required regardless of whether testing is enabled or disabled.

cdepillabout · 2019-10-25T11:07:11Z

@sevanspowell Thanks for taking the time to update this!

There are a couple things above you'll need to change before this can be merged.

Also, polysemy-plugin is marked broken, so you'll have to remove this line as well (otherwise it won't get unmarked broken next time the Haskell package set is regenerated):

nixpkgs/pkgs/development/haskell-modules/configuration-hackage2nix.yaml

Line 7963 in e8a36cd

- polysemy-plugin

peti · 2019-10-26T08:07:48Z

@sevanspowell,

The PR diff from github applies to the polysemy project as a whole, but the polysemy-plugin.src only contains the plugin part. As a result the diff is weird and I couldn't get the right fetchpatch setting to make it work.

The problem appears to be that the patch containts another directory hierarchy "polysemy-plugin/" that does not exist, obviously, in the checked-out Hackage tarball. There's multiple ways of fixing that, but the easiest one is probably to post-process the patch with stripLen:

polysemy-plugin = appendPatch (addSetupDepend super.polysemy-plugin self.cabal-doctest) (pkgs.fetchpatch {
  url = "https://github.com/polysemy-research/polysemy/pull/265.patch";
  sha256 = "19237js70chq84w7vqgvj49n6bs9lp95k13ia3xzbr1r9yyrfkhq";
  stripLen = 1;
});

Check out pkgs/build-support/fetchpatch/default.nix if you have a minute, there are many cool features that messing with patches before application. Once feature I've sometimes needed is the ability to drop certain files from the patch.

Manually altered "hackage-packages.nix" to make sure "cabal-doctest" was in the "testToolDepends" of the derivation, is there another way to do this?

See the addSetupDepend super.polysemy-plugin self.cabal-doctest bit in the snippet above.

cdepillabout · 2019-10-26T13:40:07Z

@peti Thanks for the suggestions here!

sevanspowell · 2019-10-27T22:12:36Z

Thank you @peti amd @cdepillabout. That’s super helpful! I’ll make these changes soon.

sevanspowell · 2019-10-27T23:59:02Z

RE: using fetchpatch.

That's so annoying! fetchpatch was the first tool I reached for and I figured a "stripLen" value of 1 should work. The patch failed to apply though. So I tried a couple of other values then reviewed the fetchpatch source code. I even resorted to trying to sed the patch before application but all my attempts failed. That's when I resorted to including a patch with the commit.

It's very possible I was just pulling the wrong patch from github, I'm unsure at this point. Thanks for being so helpful @peti, your suggestion worked just fine.

- polysemy-plugin was broken due to failing doctests: NixOS#71164. - I submitted a PR upstream to fix this: polysemy-research/polysemy#265. - I've applied the patch of the PR here and moved the default "polysemy" attribute to "polysemy_1_2_0_0" because polysemy-plugin requires "polysemy >= 1.2.0.0". - Move default "polysemy-zoo" attribute to "polysemy-zoo_0_6_0_1" because it is fixed by the polysemy-plugin changes and fixes issues with building "polysemy-RandomFu" and "knit-haskell". - Removed packages no longer broken from "configuration-hackage2nix.yaml". - Add cabal-doctest to setupDepends of polysemy-plugin.

peti · 2019-10-30T10:30:48Z

@GrahamcOfBorg build haskellPackages.axel haskellPackages.knit-haskell haskellPackages.polysemy-plugin haskellPackages.polysemy-RandomFu haskellPackages.polysemy-zoo

sevanspowell · 2019-10-31T09:52:13Z

I think we're pretty much ready to go.

@peti, I've followed your video on regenerating the haskell package-set for Nix and used it to mark these now fixed packages as unbroken in hackage-packages.nix. What's the typical workflow for a PR? Do we mark the packages as unbroken in configuration-hackage2nix.yaml and leave it at that, or do we submit a "hackage-packages.nix: automatic Haskell package set update" commit as part of the PR?

peti · 2019-10-31T10:26:55Z

I'd prefer if you'd just push the change to configuration-hackage2nix.yaml. The automatic generator will push the update to hackage-packages.nix soon afterwards.

sevanspowell requested review from basvandijk and cdepillabout as code owners October 25, 2019 07:10

ofborg bot added 6.topic: haskell 8.has: clean-up 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Oct 25, 2019

cdepillabout reviewed Oct 25, 2019

View reviewed changes

pkgs/development/haskell-modules/configuration-common.nix Show resolved Hide resolved

cdepillabout reviewed Oct 25, 2019

View reviewed changes

sevanspowell force-pushed the feature/fix-polysemy-plugin branch from 0e3f87e to 94455d0 Compare October 28, 2019 00:25

peti merged commit 9b87ad0 into NixOS:haskell-updates Oct 31, 2019

sevanspowell mentioned this pull request Oct 31, 2019

polysemy-plugin: Make plugin available to doctest #71164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

polysemy-plugin: fix tests #71967

polysemy-plugin: fix tests #71967

sevanspowell commented Oct 25, 2019 •

edited

sevanspowell commented Oct 25, 2019 •

edited

cdepillabout Oct 25, 2019

peti Oct 26, 2019

cdepillabout Oct 25, 2019

peti Oct 26, 2019

cdepillabout commented Oct 25, 2019

peti commented Oct 26, 2019

cdepillabout commented Oct 26, 2019

sevanspowell commented Oct 27, 2019

sevanspowell commented Oct 27, 2019

peti commented Oct 30, 2019

sevanspowell commented Oct 31, 2019

peti commented Oct 31, 2019 via email

polysemy-plugin: fix tests #71967

polysemy-plugin: fix tests #71967

Conversation

sevanspowell commented Oct 25, 2019 • edited

Motivation for this change

Things done

Notify maintainers

sevanspowell commented Oct 25, 2019 • edited

cdepillabout Oct 25, 2019

Choose a reason for hiding this comment

peti Oct 26, 2019

Choose a reason for hiding this comment

cdepillabout Oct 25, 2019

Choose a reason for hiding this comment

peti Oct 26, 2019

Choose a reason for hiding this comment

cdepillabout commented Oct 25, 2019

peti commented Oct 26, 2019

cdepillabout commented Oct 26, 2019

sevanspowell commented Oct 27, 2019

sevanspowell commented Oct 27, 2019

peti commented Oct 30, 2019

sevanspowell commented Oct 31, 2019

peti commented Oct 31, 2019 via email

sevanspowell commented Oct 25, 2019 •

edited

sevanspowell commented Oct 25, 2019 •

edited