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

reanimate-svg: disable tests & unmark broken #178511

Closed
wants to merge 1 commit into from

Conversation

schuelermine
Copy link
Contributor

Description of changes

I discovered that reanimate-svg is not actually broken, just the tests are; I believe this is since they depend on an external tool.

I unmarked it as broken and disabled tests on it.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jun 22, 2022
@cdepillabout
Copy link
Member

cdepillabout commented Jun 22, 2022

@schuelermine Thanks for sending this. A couple requests:

  • Could you rebase this on the haskell-updates branch? We normally ask all haskell-related changes to be merged into that branch. We have a separate release process for merging haskell-updates into master: haskellPackages: update stackage and hackage #178022
  • pkgs/development/haskell-modules/hackage-packages.nix generally shouldn't be edited by hand, since it is automatically generated. Instead, you can run maintainers/scripts/haskell/regenerate-hackage-packages.sh in order to regenerate it.
  • You should add dontCheck for reanimate-svg in pkgs/development/haskell-modules/configuration-common.nix. Grep through that file for dontCheck to see how other packages do it. Also make sure to add a comment for why dontCheck is necessary.
  • (nitpick) the commit title should probably start with haskellPackages.reanimate-svg: instead of just reanimate-svg:.

@sternenseemann
Copy link
Member

It'd be preferable to keep tests enabled and provide the required executable which is contained in the librsvg package.

@schuelermine
Copy link
Contributor Author

schuelermine commented Jun 22, 2022

@sternenseemann How do you do that? It seems like these packages only define Haskell dependencies

@schuelermine
Copy link
Contributor Author

@cdepillabout

Instead, you can run maintainers/scripts/haskell/regenerate-hackage-packages.sh in order to regenerate it.

Regenerate it from what?

@cdepillabout
Copy link
Member

cdepillabout commented Jun 22, 2022

Regenerate it from what?

The current hackage state, as well as a bunch of files in pkgs/development/haskell-modules that are used as inputs. Although that shouldn't matter for this PR. But definitely feel free to read through that script to see exactly what is going on.

How do you do that? It seems like these packages only define Haskell dependencies

Here's the first example I found:

# Heist's test suite requires system pandoc
heist = overrideCabal (drv: {
testToolDepends = [pkgs.pandoc];
}) super.heist;

You could add a similar override to configuration-nix.nix.

This kind of thing can sometimes be a little tricky to get right, so feel free to ask questions if you get stuck or have problems.

@schuelermine
Copy link
Contributor Author

schuelermine commented Jun 22, 2022

How do you unmark it as broken without manually editing the autogenerated file?

Oh, via configuration-hackage2nix/broken.yaml

@cdepillabout
Copy link
Member

@schuelermine

That's correct! Remove the package from configuration-hackage2nix/broken.yaml and re-run maintainers/scripts/haskell/regenerate-hackage-packages.sh.

@schuelermine
Copy link
Contributor Author

I just discovered that some tests still fail. I’m trying to build it manually right now.

@schuelermine
Copy link
Contributor Author

I think disabling the tests is OK.

The reason I say this is because one of the tests is about Arabic lettering and requires a font by the Center for Research in Urdu Language Processing. And I don’t know how you’d go about including that.

@schuelermine
Copy link
Contributor Author

OK, that might’ve been wrong, maybe that font is common. I tried the test suite in an Ubuntu VM and it worked fine.

@schuelermine
Copy link
Contributor Author

I have no clue why these tests fail. Here is a log.

https://gist.github.com/1176b5db51180aefff059dfb39b7cba9

@schuelermine
Copy link
Contributor Author

I found out what the problem is, but I don’t know why it arises.

This image

golden

when cycled through reanimate-svg is rendered instead as

cycled

…but I have no clue why this would happen here and not elsewhere. The only guess I have is that rsvg-convert itself is busted somehow.

@schuelermine
Copy link
Contributor Author

Maybe it’s the version of rsvg-convert? Nixpkgs is 2.54.3 & Ubuntu is 2.50.7.

@cdepillabout
Copy link
Member

cdepillabout commented Jun 23, 2022

@schuelermine Thanks for looking into us.

For the maintainers of the Haskell stuff in Nixpkgs, our biggest concern is that we accidentally mark a package unbroken, when really it is broken. In general, we are somewhat concerned about marking a package as dontCheck when it seems like the tests (and library itself) may actually be broken.

I don't know enough about reanimate-svg or librsvg to know how to handle this situation you're seeing. We could just mark it dontCheck and be done with it, but we ideally don't want to introduce known-broken packages to our users. After having played around with it, what is your feeling here? Does it seem like reanimate-svg may actually be broken here in Nixpkgs?

In either case, I'd really suggest creating an issue upstream. The upstream maintainer may be able to easily say something like, "oh this isn't a problem in practice, feel free to disable the tests". We could at least link to that discussion in a comment when marking reanimate-svg as dontCheck.

@schuelermine
Copy link
Contributor Author

schuelermine commented Jun 23, 2022

@cdepillabout I tested some more and I’m 99% certain that the issue is with rsvg-convert. I bundled the Nixpkgs-built rsvg-convert with nix bundle and copied it to my Ubuntu VM, and the exact same tests fail.

I also checked the source SVG files passed through reanimate-svg, and they’re the same on both systems.

This is either due to a bug or feature introduced in newer versions, or due to the Nixpkgs version of librsvg being broken itself.

@cdepillabout
Copy link
Member

Nice digging!

Sounds like even more reason to open up an issue upstream (or maybe one on both the reanimate-svg and librsvg repos).

@schuelermine
Copy link
Contributor Author

I made an issue: reanimate/reanimate-svg#42

@schuelermine
Copy link
Contributor Author

Still, there are ~50 other tests that are supposed to fail but unexpectedly succeed in Nixpkgs but not on manual build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: haskell 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants