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

add flake for easier build support #694

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

urandom2
Copy link

@urandom2 urandom2 commented Nov 7, 2022

This change adds support for nix flake based builds. This includes a dev shell required for running make targets like code generation or testing, and a package target for the output mgmt binary. We have also .gitignored the default output location, result.

Fixes #610

This change adds support for nix flake based builds. This includes a dev
shell required for running make targets like code generation or testing,
and a package target for the output mgmt binary. We have also .gitignored
the default output location, result.
Copy link
Author

@urandom2 urandom2 left a comment

Choose a reason for hiding this comment

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

no pressure to merge or review this; just got it put together based upon NixOS/nixpkgs#198963

per #610, suggestions for repo level changes are inline, but can be added to this change or broken out as a separate pr; I would suggest integrating nix build into your build process at least to verify that this logic does not break and ossify, but it is not required, and just let me know if you want a hand

Comment on lines +46 to +48
substituteInPlace Makefile --replace "/usr/bin/env " ""
substituteInPlace lang/Makefile --replace "/usr/bin/env " ""
substituteInPlace lang/types/Makefile --replace "/usr/bin/env " ""
Copy link
Author

Choose a reason for hiding this comment

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

these execute the same change I would suggest: use SHELL = bash and allow make to do PATH based resolution, as this will be more portable than assuming everybody has a /usr/bin/env

substituteInPlace Makefile --replace "/usr/bin/env " ""
substituteInPlace lang/Makefile --replace "/usr/bin/env " ""
substituteInPlace lang/types/Makefile --replace "/usr/bin/env " ""
substituteInPlace lang/types/Makefile --replace "unset GOCACHE &&" ""
Copy link
Author

Choose a reason for hiding this comment

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

I reviewed the linked upstream bug, golang/go#31843, and it is both stale/closed, and I could not trigger it during testing; can we consider it resolved and remove the override?

};
packages.default = let
pname = "mgmt";
version = "0.0.22";
Copy link
Author

Choose a reason for hiding this comment

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

unfortunately, flakes cannot consume version strings from vcs, so you will be required to manually update the string before releases, or just leave the flake as something like unstable-20221107155054 like so:

Suggested change
version = "0.0.22";
version = "unstable-${self.lastModifiedDate}";

in {
devShells.default = pkgs.mkShell {
inherit buildInputs nativeBuildInputs;
packages = [pkgs.go];
Copy link
Author

Choose a reason for hiding this comment

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

if there is anything I missed that is useful for testing/debugging/development, we should add it here

libvirt
libxml2
];
nativeBuildInputs = with pkgs; [
Copy link
Author

Choose a reason for hiding this comment

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

these are dependencies that are called during the build process explicitly

self,
}:
flake-utils.lib.eachDefaultSystem (system: let
buildInputs = with pkgs; [
Copy link
Author

Choose a reason for hiding this comment

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

these are dependencies that are linked against, think shared libraries

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package for Nixos
1 participant