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

wren: init at 0.4.0 #189295

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

wren: init at 0.4.0 #189295

wants to merge 2 commits into from

Conversation

dit7ya
Copy link
Member

@dit7ya dit7ya commented Sep 1, 2022

Description of changes

Wren is a small, fast, class-based concurrent scripting language #

Think Smalltalk in a Lua-sized package with a dash of Erlang and wrapped up in a familiar, modern syntax.

https://wren.io/

Packaging has been heavily adapted from the AUR package - https://aur.archlinux.org/packages/wren

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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Co-authored-by: Nikolay Korotkiy <[email protected]>
@dit7ya
Copy link
Member Author

dit7ya commented Sep 2, 2022

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

1 package built:
  • wren

@dit7ya dit7ya requested a review from sikmir September 2, 2022 09:54
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Hi !

Thanks for your contribution.

I've left some comments, left me know if this is clear enough.

, fetchpatch
, python3
}:
stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the finalAttrs pattern here.

The finalAttrs pattern will let you remove the rec keyword (see its implementation in Nix).

Why is this pattern preferred to rec ?

Let's take this simple code example:

mkDerivation rec {
   foo = 1;
   bar = foo + 1;
}

and then .overrideAttrs(old: { foo = 2; }), you'll get { foo = 2; bar = 2; } while with finalAttrs pattern, it would work correctly because it's a real fixed point.

Let me share a couple of useful links regarding the finalAttrs pattern:

  1. History: https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293
  2. Documentation: https://nixos.org/manual/nixpkgs/unstable/#mkderivation-recursive-attributes
  3. Recent example of implementation: https://github.com/NixOS/nixpkgs/compare/17f96f7b978e61576cfe16136eb418f74fab9952..9e6ea843e473d34d4f379b8b0d8ef0425a06defe

Feel free to reach out if you need some assistance.

owner = "wren-lang";
repo = pname;
rev = version;
sha256 = "sha256-AUb17rV07r00SpcXAOb9PY8Ea2nxtgdZzHZdzfX5pOA=";
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer hash = ... to sha256 = ... Both currently work, but the former will make it easier if the algorithm ever needs to change.

Since there could be different hashing algorithms in use, this make sense.

# otherwise fails to build
(fetchpatch {
url = "https://patch-diff.githubusercontent.com/raw/wren-lang/wren-cli/pull/136.patch";
sha256 = "sha256-/c1vbc769U/WeLGZiEdnALaooOHQcpTybfbxz+KQYUM=";
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer hash = ... to sha256 = ... Both currently work, but the former will make it easier if the algorithm ever needs to change.

Since there could be different hashing algorithms in use, this make sense.

cli =
callPackage ./cli.nix { };
in
stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the finalAttrs pattern here.

The finalAttrs pattern will let you remove the rec keyword (see its implementation in Nix).

Why is this pattern preferred to rec ?

Let's take this simple code example:

mkDerivation rec {
   foo = 1;
   bar = foo + 1;
}

and then .overrideAttrs(old: { foo = 2; }), you'll get { foo = 2; bar = 2; } while with finalAttrs pattern, it would work correctly because it's a real fixed point.

Let me share a couple of useful links regarding the finalAttrs pattern:

  1. History: https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293
  2. Documentation: https://nixos.org/manual/nixpkgs/unstable/#mkderivation-recursive-attributes
  3. Recent example of implementation: https://github.com/NixOS/nixpkgs/compare/17f96f7b978e61576cfe16136eb418f74fab9952..9e6ea843e473d34d4f379b8b0d8ef0425a06defe

Feel free to reach out if you need some assistance.

owner = "wren-lang";
repo = pname;
rev = version;
sha256 = "0w8n5lyn3wa1nmdyci0zi249w1qbq725cr1d9xsg067irq17v8k5";
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer hash = ... to sha256 = ... Both currently work, but the former will make it easier if the algorithm ever needs to change.

Since there could be different hashing algorithms in use, this make sense.


src = fetchFromGitHub {
owner = "wren-lang";
repo = pname;
Copy link
Contributor

Choose a reason for hiding this comment

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

repo = pname; is nice for DRY but creates a binding that goes too far.

See further information about this here: nix-community/nixpkgs-lint#21

python3 util/test.py
'';

installPhase = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are overriding configurePhase, buildPhase, checkPhase, installPhase or any other phase, you should not forget about explicitly running pre and post-phase hooks like their original definitions do.

It is generally expected that an appropriate pre-phase hook (e.g. preBuild) hook will run at the beginning of a phase (e.g. buildPhase) and post-phase hook (e.g. postBuild) will run at the end. Hooks are normally ran as a part of a phase so if you override a phase as a whole, you will need to add runHook hookName calls (e.g. runHook preBuild) manually.

Having phases run pre/post-phase hooks is important because many setup hooks insert their own code into them – omitting a hook might therefore prevent some setup hooks required for proper functionality of a package from running. Additionally, hooks are often inserted by developers into the package expression and by users when overriding a package using overrideAttrs. Not running them can thus cause confusion why their code is not executed.

Examples

Before

  installPhase = ''
    your commands
  '';

After

  installPhase = ''
    runHook preInstall

    your commands

    runHook postInstall
  '';

Source: https://github.com/jtojnar/nixpkgs-hammering/blob/main/explanations/missing-phase-hooks.md

python3 util/test.py
'';

installPhase = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are overriding configurePhase, buildPhase, checkPhase, installPhase or any other phase, you should not forget about explicitly running pre and post-phase hooks like their original definitions do.

It is generally expected that an appropriate pre-phase hook (e.g. preBuild) hook will run at the beginning of a phase (e.g. buildPhase) and post-phase hook (e.g. postBuild) will run at the end. Hooks are normally ran as a part of a phase so if you override a phase as a whole, you will need to add runHook hookName calls (e.g. runHook preBuild) manually.

Having phases run pre/post-phase hooks is important because many setup hooks insert their own code into them – omitting a hook might therefore prevent some setup hooks required for proper functionality of a package from running. Additionally, hooks are often inserted by developers into the package expression and by users when overriding a package using overrideAttrs. Not running them can thus cause confusion why their code is not executed.

Examples

Before

  installPhase = ''
    your commands
  '';

After

  installPhase = ''
    runHook preInstall

    your commands

    runHook postInstall
  '';

Source: https://github.com/jtojnar/nixpkgs-hammering/blob/main/explanations/missing-phase-hooks.md

@drupol drupol marked this pull request as draft August 6, 2023 11:55
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
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