Skip to content

Initial buildZigPackage#241741

Closed
IogaMaster wants to merge 9 commits intoNixOS:masterfrom
IogaMaster:zigPackages
Closed

Initial buildZigPackage#241741
IogaMaster wants to merge 9 commits intoNixOS:masterfrom
IogaMaster:zigPackages

Conversation

@IogaMaster
Copy link
Contributor

@IogaMaster IogaMaster commented Jul 5, 2023 •
edited
Loading

Description of changes

Creating a top level for zig packages as well as a simple way to build a zig package based off of nimPackages.

I tested building a Zig binary using buildZigPackage on x86_64-linux and it works.

Example of usage in a flake.

packages.default = pkgs.zig.buildZigPackage {
    name = "zig_project";
    src = ./.;
};
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@Janik-Haag Janik-Haag added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jul 5, 2023
@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 5, 2023

Working on fixing the workflow problems rn. :p

@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 5, 2023

Is it fine that the maintainers file got reformatted?

moni-dz
moni-dz suggested changes Jul 5, 2023
View reviewed changes
@moni-dz
Copy link
Contributor

moni-dz commented Jul 5, 2023

Is it fine that the maintainers file got reformatted?

No, please revert it.

figsoda
figsoda suggested changes Jul 5, 2023
View reviewed changes
Copy link
Member

@figsoda figsoda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packages should probably built with -Drelease-safe -Dcpu=baseline

@IogaMaster IogaMaster requested a review from figsoda July 5, 2023 19:32
@IogaMaster IogaMaster marked this pull request as draft July 5, 2023 19:36
@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 5, 2023

Ill do some work before finalizing

@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 Jul 5, 2023
@IogaMaster IogaMaster marked this pull request as ready for review July 5, 2023 20:03
@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 5, 2023 •
edited
Loading

I dont know how to make the changes @figsoda said about //. If I make changes it throws errors.

@IogaMaster IogaMaster requested a review from moni-dz July 5, 2023 20:07
@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 5, 2023

packages should probably built with -Drelease-safe -Dcpu=baseline

I changed this now

figsoda
figsoda reviewed Jul 5, 2023
View reviewed changes
@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 5, 2023

@figsoda How do i deal with the attribute merge? So I don't mention phases?

figsoda
figsoda reviewed Jul 5, 2023
View reviewed changes
figsoda
figsoda reviewed Jul 5, 2023
View reviewed changes
figsoda
figsoda reviewed Jul 5, 2023
View reviewed changes
figsoda
figsoda reviewed Jul 5, 2023
View reviewed changes
pkgs/top-level/zig-packages.nix Outdated
Copy link
Member

@figsoda figsoda Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this is the best approach, are we planning to package zig libraries? how would that work exactly?

Copy link
Contributor Author

@IogaMaster IogaMaster Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was planning on packing them as Zig doesn't have a package manager right now.

Copy link
Member

@figsoda figsoda Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would packaging zig libraries work, would you be able incrementally compile them?

Also, IIRC zig will have a package manager in 0.11

Copy link
Contributor Author

@IogaMaster IogaMaster Jul 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If zig will have a package manger then I don't see the point in adding packages to nixpkgs, so I will remove zigpackages

figsoda
figsoda reviewed Jul 5, 2023
View reviewed changes
figsoda
figsoda reviewed Jul 5, 2023
View reviewed changes
figsoda
figsoda reviewed Jul 5, 2023
View reviewed changes
@figsoda
Copy link
Member

figsoda commented Jul 5, 2023

cc maintainers of the zig compiler @aiotter @andrewrk @AndersonTorres

cc people that might be interested @adamcstephens @strager @winterqt

figsoda
figsoda reviewed Jul 5, 2023
View reviewed changes
@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 5, 2023

@figsoda I think this is it. What do you think?

@IogaMaster IogaMaster requested a review from figsoda July 5, 2023 23:09
figsoda
figsoda reviewed Jul 5, 2023
View reviewed changes
AndersonTorres
AndersonTorres reviewed Jul 5, 2023
View reviewed changes
AndersonTorres
AndersonTorres suggested changes Jul 8, 2023
View reviewed changes
@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 8, 2023 •
edited
Loading

Building colorstorm gives error: undefined variable 'finalAttrs' @AndersonTorres

@AndersonTorres
Copy link
Member

AndersonTorres commented Jul 8, 2023

So the finalAttrs approach is failing.
Remove it and forget for now.

@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 8, 2023 •
edited
Loading

You should finish your pr and I will rebase on top of master, then I add finalAttrs into the updated zig compiler stuff.

EDIT: Change for clarity

@IogaMaster IogaMaster mentioned this pull request Jul 8, 2023
Merged
12 tasks
roberth
roberth reviewed Jul 8, 2023
View reviewed changes
pkgs/build-support/zig/build-zig-package/default.nix Outdated
Comment on lines +5 to +10
Copy link
Member

@roberth roberth Jul 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
wrapDerivation = f:
stdenv.mkDerivation (finalAttrs:
f (lib.toFunction argsFun finalAttrs)
);
in
wrapDerivation (
changeDerivationArgs = f:
stdenv.mkDerivation (finalAttrs:
f (lib.toFunction argsFun finalAttrs)
);
in
changeDerivationArgs (

The derivation itself isn't wrapped (which is good).

pkgs/build-support/zig/build-zig-package/default.nix Outdated
Comment on lines +11 to +43
Copy link
Member

@roberth roberth Jul 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function has the shape of an overlay, if you bring finalAttrs into scope.

  • this will be helpful if you have to need the overlay to be aware of the final attrs (at some point)
  • you can document it as such

Specifically you can then say:

buildZigPackage invokes mkDerivation with your package definition and then overlays it, to add its defaults.
Hence the layers are applied in the following order:

  1. your argument to buildZigPackage
  2. zig defaults
  3. anything changed afterwards with overrideAttrs
  4. mkDerivation logic, such as the addition of finalAttrs.finalPackage

pkgs/build-support/zig/build-zig-package/default.nix Outdated
Copy link
Member

@roberth roberth Jul 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you change f to be an overlay, the composition here becomes a lot like lib.extends, except the first layer doesn't take a prev argument.
I think we could use a helper for this.

Copy link
Member

@roberth roberth Jul 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could use a helper for this.

Actually just lib.extends.
Change f to take finalAttrs: prevAttrs: and then:

-    stdenv.mkDerivation (finalAttrs:
-      f (lib.toFunction argsFun finalAttrs)
+    stdenv.mkDerivation (lib.extends f (lib.toFunction argsFun))

@roberth
Copy link
Member

roberth commented Jul 8, 2023

So the finalAttrs approach is failing. Remove it and forget for now.

I think I've read most of the conversation, but and found no reason why. What happened?

@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 8, 2023

I think we should wait for work on refactoring the zig compiler is done before I do any more work on the builder function.

@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 8, 2023

@roberth torres wants me to remove finalattrs support because if this pr is merged it would conflict with his own.

So I will wait and refactor my pr after his is merged.

@RaitoBezarius
Copy link
Member

RaitoBezarius commented Jul 8, 2023

Not too happy about another buildXYZ when it causes more pain in the long run (see rustPlatform) with respect to many subsystems of Nixpkgs (cross-compilation, etc.), I would rather see this implementation at least use more properly hooks so it's possible to do buildZigPackage with mkDerivation just by using hooks.

@AndersonTorres
Copy link
Member

AndersonTorres commented Jul 8, 2023

I think I've read most of the conversation, but and found no reason why. What happened?

Basically he tried to implement the finalAttrs support, however it is not working. So I am suggesting to implement this later.

@AndersonTorres
Copy link
Member

AndersonTorres commented Jul 8, 2023

Not too happy about another buildXYZ when it causes more pain in the long run (see rustPlatform) with respect to many subsystems of Nixpkgs (cross-compilation, etc.), I would rather see this implementation at least use more properly hooks so it's possible to do buildZigPackage with mkDerivation just by using hooks.

+1 for this!
But, how precisely it would be made? Something like we do with cmake or meson?

@figsoda
Copy link
Member

figsoda commented Jul 8, 2023 •
edited
Loading

But, how precisely it would be made? Something like we do with cmake or meson?

An easy (as in easy to design) way to do this would be to have one hook for each phase, like the ones for buildRustPackage and buildNpmPackage, so we would have something like zigBuildHook, zigCheckHook, and zigInstallHook which will override the default make ones

@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 9, 2023 •
edited
Loading

@RaitoBezarius @figsoda so you are saying rather than having custom phases I just add hooks to each phase that do the build process It's already preforming?

I will go ahead and look at the meson and cmake code to get a better idea of how things are done.

@IogaMaster IogaMaster requested a review from AndersonTorres July 9, 2023 02:39
IogaMaster and others added 9 commits July 8, 2023 20:50
@IogaMaster @figsoda
@AndersonTorres @roberth @moni-dz
Initial buildZigPackage
c10ea61 
Co-authored-by: figsoda <[email protected]>
Co-authored-by: Anderson Torres <[email protected]>
Co-authored-by: roberth <[email protected]>
Co-authored-by: fortuneteller2k <[email protected]>
@IogaMaster @AndersonTorres @figsoda
colorstorm: Init at v2.0.0
c23473b 
Co-authored-by: Anderson Torres <[email protected]>
Co-authored-by: figsoda <[email protected]>
@IogaMaster
linuxwave: update to use buildZigPackage
142f6c3
@IogaMaster @figsoda
river: update to use buildZigPackage
27f9707 
Co-authored-by: figsoda <[email protected]>
@IogaMaster
ncdu: update to use buildZigPackage
547e612
@IogaMaster
waylock: update to use buildZigPackage
b4a3df9
@IogaMaster @AndersonTorres
Documentation for buildZigPackage
277aa49 
Co-authored-by: Anderson Torres <[email protected]>
@IogaMaster
Merge branch 'NixOS:master' into zigPackages
8ca79a3
@IogaMaster
Add zig passthru to use buildZigPackage
a31e234
@AndersonTorres
Copy link
Member

AndersonTorres commented Jul 9, 2023 •
edited
Loading

I will try this!

#242397
#242565

@IogaMaster IogaMaster mentioned this pull request Jul 10, 2023
Merged
12 tasks
@IogaMaster IogaMaster closed this Jul 15, 2023
@cole-h cole-h removed the ofborg-internal-error Ofborg encountered an error label Sep 5, 2023
@uninsane uninsane mentioned this pull request Mar 11, 2024
Closed
13 tasks
@uninsane uninsane mentioned this pull request Jun 24, 2024
Closed
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roberth roberth roberth left review comments

@moni-dz moni-dz Awaiting requested review from moni-dz

@figsoda figsoda Awaiting requested review from figsoda

@adamcstephens adamcstephens Awaiting requested review from adamcstephens

@jordanisaacs jordanisaacs Awaiting requested review from jordanisaacs

@pSub pSub Awaiting requested review from pSub

@rodrgz rodrgz Awaiting requested review from rodrgz

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@AndersonTorres AndersonTorres Awaiting requested review from AndersonTorres

Assignees

@AndersonTorres AndersonTorres

Labels

8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@IogaMaster @moni-dz @figsoda @AndersonTorres @roberth @andrewrk @RaitoBezarius @cole-h @Janik-Haag