FreeBSD: Use separate files for packages#299347
Merged
Ericson2314 merged 1 commit intoNixOS:masterfrom Apr 1, 2024
Merged
Conversation
14 tasks
ofborg
bot
added
10.rebuild-darwin: 0
Member
|
@infinisil just want to check — are we ready for having non-top-level package sets using by-name? |
Ericson2314
force-pushed
the
freebsd-by-name
branch
2 times, most recently
from
3de3fa6 to
6bf738a
Compare
Member
Author
|
Converted the rest of the packages now too (not just the ones @rhelmot did). |
Ericson2314
force-pushed
the
freebsd-by-name
branch
from
6bf738a to
ac9af8f
Compare
infinisil
reviewed
Mar 28, 2024
Member
infinisil
left a comment
infinisil
left a comment
There was a problem hiding this comment.
are we ready for having non-top-level package sets using by-name?
Not really tbh, and I'd love to have some helping hands to make that a reality. I just opened NixOS/nixpkgs-vet#31 to track this and give a rough idea how to make it reality. Would appreciate some helping hands for that :)
Comment on lines
22
to
25
Member
There was a problem hiding this comment.
This function differs somewhat substantially from how pkgs/by-name works:
- No sharding (wouldn't want that here though)
- Individual files are allowed too, it doesn't have to be directories like with
pkgs/by-name. Only allowing directories was an intentional decision withpkgs/by-name, because it keeps it more consistent and easier to change. - Files are allowed to refer to files outside them, unlike
pkgs/by-namewhich forbids this. This doesn't have the benefit ofpkgs/by-nameself-containedness of packages - Recursive attribute sets can be created from directories, unlike
pkgs/by-name, which requires all attributes to be derivations. This means that it's not easily possible to act on the result, requiring hacks likerecurseIntoAttrsand co. - Similarly, it doesn't restrict
package.nixto just derivations. - And more..
While I don't think this is great, it's not necessarily a big problem here. The main point I want to make is that we shouldn't confuse pkgs/by-name with this function.
Because of that it might be best to at least rename this directory and add some explanation about how it's not like pkgs/by-name.
Member
Author
There was a problem hiding this comment.
OK yeah I didn't even realize the function was unrelated in origin. I'll rename the dir, and then I think we can do the rest as follow-up.
There are a number of packages that we ought to be able to autocall, but cannot because we need to add manual arguments just to avoid splicing. This sucks but is the right call for now --- the conclusion should be not that auto-calling is bad, but that splicing is bad. This tries to do nothing but move things around; hashes are almost unchanged. @rhelmot then has more changes to do on top of this, which will be easier to review since code will be modified in place rather than moved around and modified at the same time.
Ericson2314
force-pushed
the
freebsd-by-name
branch
from
ac9af8f to
0157c5f
Compare
Member
Author
|
OK renamed so it is no longer confused with the "real" |
Ericson2314
changed the title
Member
|
Shouldn't this result in the same drvs as before? |
Member
Author
|
@alyssais there is a slight difference in the cross-ness of a setup hook itself (based on |
Member
Author
The new way is actually more correct I think ---- the setup hook itself is not cross-compiled. |
alyssais
approved these changes
Apr 1, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of changes
There are a number of packages that we ought to be able to autocall, but cannot because we need to add manual arguments just to avoid splicing. This sucks but is the right call for now --- the conclusion should be not that by-name is bad, but that splicing is bad.
This tries to do nothing but move things around; hashes are almost unchanged. @rhelmot then has more changes to do on top of this, which will be easier to review since code will be modified in place rather than moved around and modified at the same time.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.