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.

Sign up for GitHub

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

Jump to bottom

zfs.meta.platforms: restrict to upstream-supported $TARGET_CPUs #194148

Merged
1 commit merged into from May 16, 2023
Merged

zfs.meta.platforms: restrict to upstream-supported $TARGET_CPUs #194148

1 commit merged into from May 16, 2023

Conversation

ghost
Copy link

@ghost ghost commented Oct 3, 2022 •
edited by ghost
Loading

Description of changes

ZFS on linux has a lot of manual integrations with the platform-specific build machinery inside the kernel, and this has only been performed for four architectures.

Things done

@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 Oct 3, 2022
@ghost ghost mentioned this pull request Oct 3, 2022
Closed
@alyssais
Copy link
Member

alyssais commented Oct 4, 2022 •
edited
Loading

ZFS on linux has a lot of manual integrations with the platform-specific build machinery inside the kernel, and this has only been performed for four architectures.

Sounds like it would be better to list those four platforms explicitly then?

@ghost
Copy link
Author

ghost commented Oct 4, 2022

Sounds like it would be better to list those four platforms explicitly then?

Done: #194148 (comment)

@ofborg ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 4, 2022
@ghost ghost changed the title zfs.meta.badPlatforms: add isMips64 zfs.meta.platforms: restrict to upstream-supported $TARGET_CPUs Oct 4, 2022
alyssais
alyssais requested changes Oct 5, 2022
View reviewed changes
pkgs/os-specific/linux/zfs/default.nix Outdated Show resolved Hide resolved
@ghost ghost requested a review from alyssais October 5, 2022 10:36
@ofborg ofborg bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-linux: 1-10 labels Oct 5, 2022
@ghost ghost mentioned this pull request Oct 9, 2022
Merged
13 tasks
@ofborg ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 9, 2022
@ghost
Copy link
Author

ghost commented Jan 2, 2023

Rebased.

@ghost ghost marked this pull request as draft January 2, 2023 02:43
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Jan 2, 2023
@ghost
Copy link
Author

ghost commented Jan 2, 2023 •
edited by ghost
Loading

Now that we are deeply checking meta attributes (hooray!) it is necessary to rebase upon #208686 and #208687 in order to not fail the check-meta.nix. This PR now includes some commits from those PRs in order to keep CI happy.

@ghost ghost marked this pull request as ready for review January 2, 2023 03:32
@ghost
Copy link
Author

ghost commented Jan 2, 2023

@alyssais, you marked your review comment as "resolved". Are there any other changes you would like to request?

alyssais pushed a commit that referenced this pull request Jan 11, 2023
@sternenseemann @alyssais
check-meta(hasUnsupportedPlatform): use lib.meta.availableOn
098c6b0 
`hasUnsupportedPlatform` was not updated with #37395, so it does not
understand attrsets in `meta.[bad]platforms`.  In particular,
attrsets in `meta.badPlatforms` will "fail open" and be ignored.

Let's use `lib.meta.availableOn` instead of duplicating its logic.

Thanks to @alyssais for [noticing][1].

[1][#194148 (comment)]

Co-authored-by: sternenseemann <sternenseemann@systemli.org>
@alyssais alyssais added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 11, 2023
github-actions bot pushed a commit to arcnmx/nixpkgs-lib that referenced this pull request Jan 12, 2023
@sternenseemann @alyssais
check-meta(hasUnsupportedPlatform): use lib.meta.availableOn
1cda0ef 
`hasUnsupportedPlatform` was not updated with #37395, so it does not
understand attrsets in `meta.[bad]platforms`.  In particular,
attrsets in `meta.badPlatforms` will "fail open" and be ignored.

Let's use `lib.meta.availableOn` instead of duplicating its logic.

Thanks to @alyssais for [noticing][1].

[1][NixOS/nixpkgs#194148 (comment)]

Co-authored-by: sternenseemann <sternenseemann@systemli.org>
github-actions bot pushed a commit to nix-community/nixpkgs.lib that referenced this pull request Jan 15, 2023
@sternenseemann @alyssais
check-meta(hasUnsupportedPlatform): use lib.meta.availableOn
a5286c6 
`hasUnsupportedPlatform` was not updated with #37395, so it does not
understand attrsets in `meta.[bad]platforms`.  In particular,
attrsets in `meta.badPlatforms` will "fail open" and be ignored.

Let's use `lib.meta.availableOn` instead of duplicating its logic.

Thanks to @alyssais for [noticing][1].

[1][NixOS/nixpkgs#194148 (comment)]

Co-authored-by: sternenseemann <sternenseemann@systemli.org>
@ghost
Copy link
Author

ghost commented Jan 24, 2023

Fixed merge conflict.

@github-actions github-actions bot removed the 6.topic: stdenv Standard environment label Jan 24, 2023
@ofborg ofborg bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-linux: 1-10 labels Jan 24, 2023
adamcstephens pushed a commit to adamcstephens/nixpkgs that referenced this pull request Jan 25, 2023
@sternenseemann @adamcstephens
check-meta(hasUnsupportedPlatform): use lib.meta.availableOn
8abe455 
`hasUnsupportedPlatform` was not updated with NixOS#37395, so it does not
understand attrsets in `meta.[bad]platforms`.  In particular,
attrsets in `meta.badPlatforms` will "fail open" and be ignored.

Let's use `lib.meta.availableOn` instead of duplicating its logic.

Thanks to @alyssais for [noticing][1].

[1][NixOS#194148 (comment)]

Co-authored-by: sternenseemann <sternenseemann@systemli.org>
@ghost ghost mentioned this pull request Apr 24, 2023
Closed
12 tasks
@ghost
Copy link
Author

ghost commented Apr 24, 2023

Rebased.

1 similar comment
@ghost
Copy link
Author

ghost commented May 15, 2023

Rebased.

@ghost ghost requested a review from alyssais May 15, 2023 08:15
@ghost ghost merged commit ede53c3 into NixOS:master May 16, 2023
github-actions bot pushed a commit to nix-community/nixpkgs.lib that referenced this pull request May 31, 2023
@sternenseemann @alyssais
check-meta(hasUnsupportedPlatform): use lib.meta.availableOn
63650f7 
`hasUnsupportedPlatform` was not updated with #37395, so it does not
understand attrsets in `meta.[bad]platforms`.  In particular,
attrsets in `meta.badPlatforms` will "fail open" and be ignored.

Let's use `lib.meta.availableOn` instead of duplicating its logic.

Thanks to @alyssais for [noticing][1].

[1][NixOS/nixpkgs#194148 (comment)]

Co-authored-by: sternenseemann <sternenseemann@systemli.org>
@Majiir
Copy link
Contributor

Majiir commented Jun 23, 2023

@amjoseph-nixpkgs Can you provide some more detail on what breaks when building ZFS on platforms other than these five?

I've been running ZFS on armv7l-linux (cross-compiled from x86_64-linux) for ~6 months now. After this PR landed, I patched my nixpkgs to add isArmv7 to this list. I can also build a minimal ISO with that patch. I'm hoping we can find a more relaxed check.

@ghost
Copy link
Author

ghost commented Jun 26, 2023

@amjoseph-nixpkgs Can you provide some more detail on what breaks when building ZFS on platforms other than these five?

It's explained in the comment which this PR added. See the link to the ZFS source code.

I've been running ZFS on armv7l-linux (cross-compiled from x86_64-linux) for ~6 months now. After this PR landed, I patched my nixpkgs to add isArmv7 to this list. I can also build a minimal ISO with that patch. I'm hoping we can find a more relaxed check.

Great, submit a PR.

The only reason I wrote this PR is that forcibly breaking ZFS on platforms where it won't build is the only way to get a NixOS ISO to build for those platforms, because NixOS forcibly enables ZFS:

nixpkgs/nixos/modules/tasks/filesystems/zfs.nix

Lines 214 to 215 in e3cbd65

enabled = mkOption {
readOnly = true;

@ghost
Copy link
Author

ghost commented Jun 26, 2023 •
edited by ghost
Loading

BTW, I confirmed that zfs does in fact build on armv7. So we should add it to the allowed platforms.

Or, even better, go back to enumerating the badPlatforms (the platforms on which this does not build) instead of platforms (the platforms on which it does build), like the original version of this PR did.

Or, best of all, let people turn off ZFS.

@ghost ghost deleted the pr/zfs/mips-bad branch June 26, 2023 12:02
@Majiir Majiir mentioned this pull request Aug 22, 2023
Merged
3 tasks
@Artturin Artturin mentioned this pull request Jan 29, 2024
Open
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@piegamesde piegamesde Awaiting requested review from piegamesde

@edolstra edolstra Awaiting requested review from edolstra

@nbp nbp Awaiting requested review from nbp

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
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
@alyssais @Majiir