-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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: mark linux-only packages #122806
haskellPackages: mark linux-only packages #122806
Conversation
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.
Thanks for doing this, I was meaning to do more towards this direction as well!
Can you please rebase this onto haskell-updates
? It makes testing and stabilizing haskell changes when everything happens on one branch.
@@ -126,6 +126,10 @@ rec { | |||
*/ | |||
dontDistribute = drv: overrideCabal drv (drv: { hydraPlatforms = []; }); | |||
|
|||
/* overridePlatforms changes the supported platforms for a package. |
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.
Can you add an example and type signature like this: https://github.com/tazjin/nixdoc#comment-format?
@@ -126,6 +126,10 @@ rec { | |||
*/ | |||
dontDistribute = drv: overrideCabal drv (drv: { hydraPlatforms = []; }); | |||
|
|||
/* overridePlatforms changes the supported platforms for a package. | |||
*/ | |||
overridePlatforms = drv: fn: overrideCabal drv (drv: { platforms = fn pkgs.lib.platforms; }); |
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.
Not sure if I'm too happy with using a function here — there is really no need to as there is only one platforms
set (as opposed to other cases where we use this type of API like ghcWithPackages
where there are different sets the function could take packages from).
I guess the shortcut is nice, but I fear we introduce a unnecessary and potentially unintuitive API here. Interested to hear other opinions.
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 disagree, and it was definitely for convenience only. My original plan was to see if I could accomplish this with even less boilerplate, as maybe platformOverrides = { honk = platforms.linux; ... }
, but I went for something like this to keep the diff small this close to release.
So, we can of course inline it all:
{
honk = overridePlatforms super.honk pkgs.lib.platforms.linux;
}
Or maybe introduce a variable?
let
inherit (pkgs.lib) platforms;
in {
honk = overridePlatforms super.honk platforms.linux;
}
Though this file does not contain any yet, and I'm not sure if it's desirable.
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 pushed a version that inlines everything.
I also rebased on the merge-base of haskell-updates
and master
. Is that sufficient for testing, or do we need to be on the latest haskell-updates
?
The reason I'm targeting master
is because of ZHF, and the next haskell-updates
merge is scheduled post-branchoff: #122719
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.
No, the next merge is not supposed to be happening past branch-off. The date given there is the usual time window of two weeks.
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.
Alright, in that case, I switched target branch and rebased on current haskell-updates
. 👍
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.
These settings belong into configuration-hackage2nix.yaml.
I disagree. The |
I am a bit torn on the issue. I feel like if this is a only a temporary fix it would be okay to do it via the yaml file (I mean there won‘t be any new platforms added before branch off, will they?) And if this is not temporary we should also do it via the yaml file but need to change hackage2nix a bit. Introducing this solution will introduce an inconsistent split. Between some platform being configured via yaml and some via nix. |
I want this as an intermediate solution at the very least because it allows us to clean up the In the future, I'd like to rework how platforms work in |
621fb5d
to
6eee96e
Compare
6eee96e
to
d2a3ac9
Compare
I think I agree with @maralorn here. If this is just a temporary fix, then it doesn't really matter how we do it for the 21.05 release. However, in the long run, I'd like to go with a solution that is automatable (well, really, just some solution that makes us do minimal work). Up until now, most of our automatable solutions have been based on yaml files. @sternenseemann Do you have some sort of long-term vision for how platform-related stuff should work in the Haskell infrastructure? I guess I'm mainly interested in what exactly the three of us will have to do for this. I don't personally want to deal with problems on multiple platforms more than my current level of almost-completely-ignoring-it. (Although don't get me wrong, I do like our current approach of trying to push more of the burden on drive-by-contributors.) |
I'm not familiar with Haskell, nor am I a big Nix contributor, but my 2 cents: I don't think this can be automated, unless upstream lists this somewhere in package metadata. But I don't see anything on hackage or in the cabal files of these packages. So pushing this to drive-by contributions (and efforts like ZHF) is probably the right solution for now. What could help longer term is more automated nixpkgs-review of PRs on Darwin machines, like @r-rmcgibbo currently does for x86_64-linux and aarch64-linux. (Though that sort of review is currently not a requirement for merging.) Right now, listing the two Darwin platforms in the existing
I think a long-term solution should at least:
|
d2a3ac9
to
bc2528b
Compare
I pushed a version that lists these in the YAML just for I cannot test it, though, because
I created a case-sensitive APFS volume for this, so that should not be the problem. I also don't believe the LC_ALL setting is the cause, though it may be a separate issue. (Seems like |
Thanks, this looks good to merge in for now at least. |
Motivation for this change
ZHF: #122042
cc @NixOS/nixos-release-managers, @NixOS/haskell
A small subset of Haskell packages that are currently failing on Darwin are simply not supported on Darwin by design, but not marked as such. This PR adds a helper and starts a list of these packages. I also included
cpuid
andhsignal
under this helper umbrella, because they do a similar thing, but for x86-only.Haskell in Nix has the
unsupported-platforms
list, but it's flexibility is limited, only allowing to eliminate specific platforms. For example, marking a package as not supported on Darwin would've worked for a while withx86_64-darwin
, but then we gotaarch64-darwin
. Similarly, marking a package as Linux-only is impossible: there is no oppositesupported-platforms
, plus we're continuously adding Linux platforms likeriscv64-linux
.I think fixing this properly is outside the scope of ZHF and the upcoming release, and hopefully this stop-gap solution is okay for now.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)