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.gi-webkit2: Fix build #91755

Merged
merged 1 commit into from Jul 3, 2020
Merged

Conversation

@maralorn
Copy link
Contributor

maralorn commented Jun 29, 2020

Motivation for this change

Fixing some package builds.

I strongly believe, that all overrides in configuration-common.nix should have a mechanism to not go stale.
That‘s why I am experimenting with an assert here.

Things done
  • 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.
@cdepillabout
Copy link
Member

cdepillabout commented Jun 29, 2020

Let me see if I understand what is going on here:

  1. haskell-gi is in stackage LTS-16, so it is kept back at version 0.23.
  2. gi-javascriptcore is not in stackage LTS-16, so it is just pulled from the latest version on hackage, which is currently 4.0.22.
  3. gi-javascriptcore-4.0.22 requires haskell-gi-0.24.
  4. However, gi-javascriptcore doesn't actually need haskell-gi-0.24, it works fine with haskell-gi-0.23.

This means that in order to build gi-javascriptcore in nixpkgs currently, we either need a whole bunch of overrides, or we just need to jailbreak gi-javascriptcore to build with an older version of haskell-gi?

The code in this PR is as follows:

gi-javascriptcore =
  # Remove these jailbreaks, when assert fails.
  assert (pkgs.lib.versionOlder super.haskell-gi-base.version "0.24");
  doJailbreak super.gi-javascriptcore;

This means that gi-javascriptcore will fail to build when the haskell-gi version is 0.24 or newer.


I don't have a strong opinion for or against this, but I do have a couple concerns:

  1. If there was a revision to gi-javascriptcore-4.0.22 on Hackage, then this assert/doJailbreak could potentially not be needed, even if haskell-gi-base is still 0.23. This assert doesn't capture this possibility.

  2. I wonder how an expression like this would deal with haskellPackages getting some overrides applied. For instance, if haskell-gi-base was overridden to a more recent version, then I guess this would still work?

    However, what if gi-javascriptcore was overridden to be an older version? Would that be okay as well?

    I guess what I'm mainly worried about is that doJailbreak is able to be disabled with the dontJailbreak function, but I imagine it is not as easy to remove the assert without directly editing the source file.

  3. It might be nice to have a version of doJailbreak that specifically includes this assert. Something you could call like this:

    doJailbreakFor "haskell-gi-base" "older" "0.24" super.gi-javascriptcore
@maralorn
Copy link
Contributor Author

maralorn commented Jun 29, 2020

Let me see if I understand what is going on here:

1. `haskell-gi` is in stackage LTS-16, so it is kept back at version `0.23`.

2. `gi-javascriptcore` is not in stackage LTS-16, so it is just pulled from the latest version on hackage, which is currently `4.0.22`.

3. `gi-javascriptcore-4.0.22` requires `haskell-gi-0.24`.

4. However, `gi-javascriptcore` doesn't _actually_ need `haskell-gi-0.24`, it works fine with `haskell-gi-0.23`.

This means that in order to build gi-javascriptcore in nixpkgs currently, we either need a whole bunch of overrides, or we just need to jailbreak gi-javascriptcore to build with an older version of haskell-gi?

Yes, that is exactly right.

The code in this PR is as follows:

gi-javascriptcore =
  # Remove these jailbreaks, when assert fails.
  assert (pkgs.lib.versionOlder super.haskell-gi-base.version "0.24");
  doJailbreak super.gi-javascriptcore;

This means that gi-javascriptcore will fail to build when the haskell-gi version is 0.24 or newer.

I don't have a strong opinion for or against this, but I do have a couple concerns:

1. If there was a revision to `gi-javascriptcore-4.0.22` on Hackage, then this assert/doJailbreak could potentially not be needed, even if `haskell-gi-base` is still `0.23`.  This `assert` doesn't capture this possibility.

That is right. But my primary concern is to not have this jailbreak linger here forever.

2. I wonder how an expression like this would deal with `haskellPackages` getting some overrides applied.  For instance, if `haskell-gi-base` was overridden to a more recent version, then I guess this would still work?
   However, what if `gi-javascriptcore` was overridden to be an older version?  Would that be okay as well?
   I guess what I'm mainly worried about is that `doJailbreak` is able to be disabled with the `dontJailbreak` function, but I imagine it is not as easy to remove the `assert` without directly editing the source file.

That is a valid concern I hadn‘t thought about.

  1. I think updating haskell-gi-base to a newer version would work because it uses super. So the assert happens against the old package. If someone where to fork nixpkgs and bump the version in hackage-packages.nix they would run into this assert, but that’s intentional.
  2. I think downgrading gi-javascriptcore to an older version with an override might fail late in the build because of the jailbreak. But I don‘t think the assert increases the severity of that problem.

So my summary is. I agree that it might be a problem that you cannot override the assert, but I can‘t think of a situation where the assert would fail without that being intended.

3. It might be nice to have a version of `doJailbreak` that specifically includes this `assert`.  Something you could call like this:
   ```nix
   doJailbreakFor "haskell-gi-base" "older" "0.24" super.gi-javascriptcore
   ```

I am not a big fan of the stringly typed "older" in there, but yeah, that might be a good idea. In general I think it would be great to annotate overrides in configuration-common.nix with informations/asserts so that they can‘t get stale. But I haven‘t thought about the whole design space yet.

@maralorn maralorn force-pushed the maralorn:fix-gi-webkit2 branch from b10019f to df73cc3 Jun 29, 2020
@maralorn
Copy link
Contributor Author

maralorn commented Jun 29, 2020

@GrahamcOfBorg build haskellPackages.reflex-dom

@cdepillabout
Copy link
Member

cdepillabout commented Jun 30, 2020

I agree that it might be a problem that you cannot override the assert, but I can‘t think of a situation where the assert would fail without that being intended.

Yeah, that's sort of the conclusion I came to as well. In the example in this PR here, whether you overrode haskell-gi-base to an older or newer version, or you overrode gi-javascriptcore to an older or newer version, I couldn't come up with a time when you would accidentally trigger the alert.

(Although I do think that we should provide a separate doJailbreakFor-like function, since most people would probably pass self.haskell-gi-base to the assert, instead of the correct super.haskell-gi-base.)

I am not a big fan of the stringly typed "older" in there, but yeah, that might be a good idea.

Ah, yeah, I completely agree.


Ping @peti to see how he feels about this.

@peti peti merged commit 3c9eb1b into NixOS:haskell-updates Jul 3, 2020
16 checks passed
16 checks passed
haskellPackages.reflex-dom on aarch64-linux Timed out, unknown build status
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df73cc3"; rev="df73cc36317106451d7a90dd5733bb5a1fd52460"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df73cc3"; rev="df73cc36317106451d7a90dd5733bb5a1fd52460"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df73cc3"; rev="df73cc36317106451d7a90dd5733bb5a1fd52460"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df73cc3"; rev="df73cc36317106451d7a90dd5733bb5a1fd52460"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df73cc3"; rev="df73cc36317106451d7a90dd5733bb5a1fd52460"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df73cc3"; rev="df73cc36317106451d7a90dd5733bb5a1fd52460"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df73cc3"; rev="df73cc36317106451d7a90dd5733bb5a1fd52460"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
haskellPackages.reflex-dom on x86_64-linux Success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.