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

haskellPackages.graphql: remove asserts #166425

Closed
wants to merge 1 commit into from

Conversation

avieth
Copy link
Contributor

@avieth avieth commented Mar 30, 2022

Description of changes

With the asserts in place, it becomes more awkward to, for example,
filter the packages in the haskell package set, because forcing graphql
causes an error. Seems to me there's nothing wrong in theory with forcing
this derivation, even if the hspec and parser-combinators versions are no
good. The problem should only arise when trying to build 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.05 Release Notes (or backporting 21.11 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.

With the asserts in place, it becomes more awkward to, for example,
filter the packages in the haskell package set, because forcing graphql
causes an error. Seems to me there's nothing wrong in theory with forcing
this derivation, even if the hspec and parser-combinators versions are no
good. The problem should only arise when trying to build it.
@sternenseemann
Copy link
Member

How are you hitting these asserts? The intention behind them is that when they fire, they ought to be cleaned up altogether as the fix guarded by them is no longer necessary. This wouldn't be as apparent with meta.broken.

If you are switching out versions and want to skip the configuration-common.nix overlay, you can override configurationCommon = (self: super: {});.

@avieth
Copy link
Contributor Author

avieth commented Mar 30, 2022

How are you hitting these asserts?

I apply an overlay which picks a new hspec, and then check every member of the package set, causing the assertion to fail and the program to crash.

The intention behind them is that when they fire, they ought to be cleaned up altogether as the fix guarded by them is no longer necessary.

Not sure I follow this. Do you mean I ought to override graphql too? Wouldn't be such a nice workaround IMO since graphql is not even used in my application. I don't need to build it.

@sternenseemann
Copy link
Member

Not sure I follow this. Do you mean I ought to override graphql too?

Point being, that this is a tool used by us upstream maintainers to let us know when we can remove an override in configuration-common.nix. There should of course not be a way for downstream users to trigger this by accident.

I apply an overlay which picks a new hspec

I see. The problem is that we assert a property about another package in this case which is bound to bite someone like you when they start switching out packages. asserting something over itself is okay (since then downstream overrides would also remove the assert when overriding). I'll remove asserts like this and try to communicate the issue, so it doesn't happen again.

and then check every member of the package set, causing the assertion to fail and the program to crash.

As a workaround, I can recommend doing a lib.filterAttrs (_: v: (builtins.tryEval (v.outPath or null)).success) haskellPackages beforehand.

sternenseemann added a commit that referenced this pull request Mar 30, 2022
We should only make use of asserts to assert a property about the
*current* attribute in order to make it possible for downstream users to
change versions of packages: When a downstream user changes the package
an attribute points to, the assert is removed as the attribute is
swapped out, so asserting something about itself is okay. However, when
it asserts a property about another package, changing that other package
may break the package unexpectedly, with no better workaround then
passing in an empty `configurationCommon` overlay.

See also: #166425
@sternenseemann
Copy link
Member

Addressed as part of #160733

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

2 participants