-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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.ghcjs-dom: build on js backend of ghc 9.8 #309650
Conversation
I assume on 9.6 the FFI support wasn't there yet as the build fails with
|
] ++ lib.optionals stdenv.targetPlatform.isGhcjs [ | ||
(configurationJS { inherit pkgs haskellLib; }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to add this before, alongside the other platformConfigurations
, but the overrides would get lost as configuration-ghc-9.8.nix
(passed in compilerConfig
) would then reset to null
or overwrite due to picking the most-recent hackage version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s unfortunate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t get it. there do not seem to be conflicting overrides for the packages in ghcjs-9.x.nix after the changes you are making. Are you sure this doesn’t work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this doesn’t work?
Confirmed to be working now. I think the problem is that I originally had
aeson = disableLibraryProfiling super.aeson;
which was being replaced by the flat aeson
override in the 9.8 bit but now we disable profiling globally for JS flavors so this is no longer a problem.
8c61a34 should simplify this PR a lot! I'm wondering if we even need a special file for the js backend at all. Most of these packages wouldn't build with normal GHCs anyways, so we can apply the overrides in the normal configuration-nix.nix? Or do you think there's an advantage in having them conditional? The disadvantage of this is that eval regressions won't be caught since we can't test the entire set on Hydra… |
Huh, how? DId you mean the With this change, the
I think the change also requires removing
so that it builds on That in turn makes
to
so we probably should set its supported platforms, but that needs the call2nix release. Edit: Done |
564ef5d
to
50cc576
Compare
Anything left to do here? |
Thank you very much! Any explanation regarding the white space changes in hackage-packages.nix? I left one other comment. Apart from that this looks good to me. Ah, do we already have enough hydra jobs for this? |
@@ -36,6 +37,8 @@ let | |||
compilerConfig | |||
packageSetConfig | |||
overrides | |||
] ++ lib.optionals stdenv.hostPlatform.isGhcjs [ | |||
(configurationJS { inherit pkgs haskellLib; }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if we could just use platformConfigurations
for this instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. This now works, as per: #309650 (comment)
Oops, fixed. |
Probably not, we should expand that, but that can probably be done in a follow up change. |
Description of changes
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.