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.

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

Initial buildZigPackage #241741

Closed
wants to merge 9 commits into from
Closed

Initial buildZigPackage #241741

wants to merge 9 commits into from

Conversation

IogaMaster
Copy link
Contributor

@IogaMaster IogaMaster commented Jul 5, 2023

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.

@IogaMaster
Copy link
Contributor Author

Working on fixing the workflow problems rn. :p

@IogaMaster
Copy link
Contributor Author

Is it fine that the maintainers file got reformatted?

@moni-dz
Copy link
Contributor

moni-dz commented Jul 5, 2023

Is it fine that the maintainers file got reformatted?

No, please revert it.

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

maintainers/maintainer-list.nix Outdated Show resolved Hide resolved
@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

Ill do some work before finalizing

@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

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

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

I changed this now

@IogaMaster
Copy link
Contributor Author

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

@@ -0,0 +1,10 @@
{ lib, pkgs, stdenv, newScope, zig, fetchFromGitHub, buildPackages }:

lib.makeScope newScope (self:
Copy link
Member

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

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

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

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
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

@IogaMaster
Copy link
Contributor Author

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

@IogaMaster IogaMaster requested a review from figsoda July 5, 2023 23:09
pkgs/top-level/zig-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/zig-packages.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/zig/0.10.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/zig/0.10.nix Show resolved Hide resolved
pkgs/development/compilers/zig/0.9.1.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/zig/0.9.1.nix Show resolved Hide resolved
pkgs/applications/misc/colorstorm/default.nix Show resolved Hide resolved
pkgs/applications/misc/colorstorm/default.nix Show resolved Hide resolved
@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 8, 2023

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

@AndersonTorres
Copy link
Member

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

@IogaMaster
Copy link
Contributor Author

IogaMaster commented Jul 8, 2023

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
12 tasks
Comment on lines +5 to +10
wrapDerivation = f:
stdenv.mkDerivation (finalAttrs:
f (lib.toFunction argsFun finalAttrs)
);
in
wrapDerivation (
Copy link
Member

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).

Comment on lines +11 to +43
{ strictDeps ? true
, nativeBuildInputs ? [ ]
, meta ? { }
, ...
}@attrs:

attrs // {
inherit strictDeps;
nativeBuildInputs = [ zig ] ++ nativeBuildInputs;

buildPhase = attrs.buildPhase or ''
runHook preBuild
export ZIG_GLOBAL_CACHE_DIR=$(mktemp -d)
zig build -Drelease-safe -Dcpu=baseline $zigBuildFlags
runHook postBuild
'';

checkPhase = attrs.checkPhase or ''
runHook preCheck
zig build test
runHook postCheck
'';

installPhase = attrs.installPhase or ''
runHook preInstall
zig build -Drelease-safe -Dcpu=baseline $zigBuildFlags --prefix $out install
runHook postInstall
'';

meta = {
inherit (zig.meta) platforms;
} // meta;
}
Copy link
Member

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

let
wrapDerivation = f:
stdenv.mkDerivation (finalAttrs:
f (lib.toFunction argsFun finalAttrs)
Copy link
Member

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

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

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

@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

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

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

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

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

@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 and others added 9 commits July 8, 2023 20:50
Co-authored-by: figsoda <figsoda@pm.me>
Co-authored-by: Anderson Torres <torres.anderson.85@protonmail.com>
Co-authored-by: roberth <robert@roberthensing.nl>
Co-authored-by: fortuneteller2k <lythe1107@gmail.com>
Co-authored-by: Anderson Torres <torres.anderson.85@protonmail.com>
Co-authored-by: figsoda <figsoda@pm.me>
Co-authored-by: figsoda <figsoda@pm.me>
Co-authored-by: Anderson Torres <torres.anderson.85@protonmail.com>
@AndersonTorres
Copy link
Member

AndersonTorres commented Jul 9, 2023

I will try this!

#242397
#242565

@IogaMaster IogaMaster mentioned this pull request Jul 10, 2023
12 tasks
@IogaMaster IogaMaster closed this Jul 15, 2023
@uninsane uninsane mentioned this pull request Mar 11, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants