Skip to content

haskellPackages.graphql: remove asserts#166425

Closed
avieth wants to merge 1 commit intoNixOS:masterfrom
avieth:avieth/graphql-unassert
Closed

haskellPackages.graphql: remove asserts#166425
avieth wants to merge 1 commit intoNixOS:masterfrom
avieth:avieth/graphql-unassert

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.

@avieth
haskellPackages.graphql: remove asserts
108a2b3 
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.
@github-actions github-actions bot added the 6.topic: haskell General-purpose, statically typed, purely functional programming language label Mar 30, 2022
@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 Mar 30, 2022
@sternenseemann
Copy link
Member

sternenseemann commented Mar 30, 2022

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

sternenseemann commented Mar 30, 2022

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
@sternenseemann
haskellPackages.graphql: convert assert on hspec into warning
b0d3044 
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

sternenseemann commented Mar 30, 2022

Addressed as part of #160733

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

Reviewers

@cdepillabout cdepillabout Awaiting requested review from cdepillabout

@sternenseemann sternenseemann Awaiting requested review from sternenseemann

@maralorn maralorn Awaiting requested review from maralorn

@expipiplus1 expipiplus1 Awaiting requested review from expipiplus1

Assignees

No one assigned

Labels

6.topic: haskell General-purpose, statically typed, purely functional programming language 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@avieth @sternenseemann