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

git-gamble: init at 2.9.0 #332592

Merged
merged 1 commit into from
Aug 13, 2024
Merged

git-gamble: init at 2.9.0 #332592

merged 1 commit into from
Aug 13, 2024

Conversation

pinage404
Copy link
Contributor

@pinage404 pinage404 commented Aug 5, 2024

Description of changes

I updated / improved some packages i maintain, then i added a package, if i have to split, just tell me =)

Tool that blends TDD (Test Driven Development) + TCR (test && commit || revert) to make sure to develop the right thing 😌, baby step by baby step 👶🦶
https://git-gamble.is-cool.dev/

When a package already have a flake.nix which expose packages.<system>.<package-name>, what is the recommended way of packaging ? Do there is a way to reuse the flake output in nixpkgs ?

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@pinage404 pinage404 changed the title vscode-runner: init at 1.6.1 git-gamble: init at 2.9.0 Aug 5, 2024
@pinage404
Copy link
Contributor Author

Result of nixpkgs-review pr 332592 run on x86_64-linux 1

1 package built:
  • git-gamble

@pinage404
Copy link
Contributor Author

@afh you liked this PR #331862, maybe you will like this one

Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

Thanks for pinging me on this, @pinage404.

IMO this PR should only contain commits related to adding the initial version of the git-gamble package.

Apart from that please find below a few more suggestions for improvement.

pkgs/by-name/gi/git-gamble/package.nix Outdated Show resolved Hide resolved
Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

Thanks for pinging me on this, @pinage404.

IMO this PR should only contain commits related to adding the initial version of the git-gamble package.

Apart from that please find below a few more suggestions for improvement.

pkgs/by-name/gi/git-gamble/package.nix Outdated Show resolved Hide resolved
@pinage404
Copy link
Contributor Author

When a package already have a flake.nix which expose packages.<system>.<package-name>

What is the recommended way of packaging ?

Do there is a way to reuse the flake output in nixpkgs ?

@pbsds
Copy link
Member

pbsds commented Aug 12, 2024

Do there is a way to reuse the flake output in nixpkgs ?

Not without import-from-derivation (IFD) which is disallowed in nixpkgs.

];
postInstall = ''
wrapProgram $out/bin/git-gamble \
--prefix PATH : "${lib.makeBinPath [ git ]}"
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 wonder which is better between git and gitMinimal ?

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the package definition of gitMinimal I'd say it's a good choice to use here, unless git-gamble requires support for python, perl or pcre2 in git…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done !

Thanks for the quick response !

@pinage404
Copy link
Contributor Author

Not without import-from-derivation (IFD) which is disallowed in nixpkgs.

Why is it disallowed ?

@pbsds
Copy link
Member

pbsds commented Aug 13, 2024

Import From Derivation (IFD) is disallowed in Nixpkgs for performance reasons.
A nix build is split into two phases, evaluation and build. Evaluation is where the nix code is parsed and turned into derivation files, and the build executes in those derivation files and realize or substitute them into store paths.
The problem with IFD is that the evaluation runs into a stop when trying to import more nix code from the store, and has to build that store path before being able to continue the evaluation.
Not only is this conceptually a blocking operation, but the cppnix runtime is very poorly optimized to schedule between and evaluations and builds.
Hydra regularily evaluate the entire nixpkgs package set, and sequential builds during evaluation would increase evaluation times to the points where it become impractical.

@pbsbot
Copy link

pbsbot commented Aug 13, 2024

Result of nixpkgs-review pr 332592 run on x86_64-linux 1

1 package built:
  • git-gamble

@pbsds pbsds merged commit 6e5c9a1 into NixOS:master Aug 13, 2024
25 of 26 checks passed
@afh
Copy link
Member

afh commented Aug 13, 2024

Thank you for providing insightful context, @pbsds, very helpful and much appreciated 👍

@pinage404 pinage404 deleted the git-gamble-init branch August 13, 2024 20:35
@pinage404
Copy link
Contributor Author

It seems weird to me that flake are not well integrated into the nix build ecosystem

Thanks for the explanations =D

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.

4 participants