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

Jump to bottom

beets.plugins.{alternative,copyartifacts}: fix evaluation #109679

Closed
wants to merge 1 commit into from
Closed

beets.plugins.{alternative,copyartifacts}: fix evaluation #109679

wants to merge 1 commit into from

Conversation

mdorman
Copy link
Contributor

@mdorman mdorman commented Jan 18, 2021 •
edited

Motivation for this change

Commit 8c5d371 apparently did not confirm that all its alterations resulted in evaluatable expressions, meaning that these files became unevaluatable:

error: undefined variable 'lib' at
/var/nixup/nixpkgs/pkgs/tools/audio/beets/plugins/alternatives.nix:22:21

The fix is simply to make sure the required value is available as a function argument.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@mdorman
beets.plugins.{alternative,copyartifacts}: fix evaluation
02afbaf 
Commit 8c5d371 apparently did not
confirm that all its alterations resulted in evaluatable expressions,
meaning that these files became unevaluatable:

    error: undefined variable 'lib' at
    /var/nixup/nixpkgs/pkgs/tools/audio/beets/plugins/alternatives.nix:22:21

The fix is simply to make sure the required value is available as a
function argument.

Verified through evaluation.
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Jan 18, 2021 •
edited

Commit 8c5d371 apparently did not confirm that all its alterations resulted in evaluatable expressions, meaning that these files became unevaluatable

Thats because it is not evaluated by ofborg.

@siraben

SuperSandro2000
SuperSandro2000 requested changes Jan 18, 2021
View reviewed changes
pkgs/tools/audio/beets/plugins/copyartifacts.nix
@@ -1,4 +1,4 @@
{ stdenv, fetchFromGitHub, beets, pythonPackages, glibcLocales }:
{ lib, stdenv, fetchFromGitHub, beets, pythonPackages, glibcLocales }:
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
{ lib, stdenv, fetchFromGitHub, beets, pythonPackages, glibcLocales }:
{ lib, fetchFromGitHub, beets, pythonPackages, glibcLocales }:

pkgs/tools/audio/beets/plugins/alternatives.nix
@@ -1,4 +1,4 @@
{ stdenv, fetchFromGitHub, beets, pythonPackages }:
{ lib, stdenv, fetchFromGitHub, beets, pythonPackages }:
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
{ lib, stdenv, fetchFromGitHub, beets, pythonPackages }:
{ lib, fetchFromGitHub, beets, pythonPackages }:

@grahamc
Copy link
Member

grahamc commented Jan 19, 2021 •
edited

Thats because it is not evaluated by ofborg.

this is a bug in the beets expression: hydra doesn't evaluate or build them and ofborg tries to evaluate everything Hydra does. Please, for the long term sustainability of the project, don't just move past these sorts of issues and help us find systemic fixes.

I think we need in the list of all packages:

beetsPlugins = lib.recurseIntoAttrs beets.plugins;

@aszlig
Copy link
Member

aszlig commented Feb 2, 2021

@mdorman: Can you please rebase against master? This should also address the suggestions made by @SuperSandro2000.

@aszlig
Copy link
Member

aszlig commented Feb 2, 2021

I think we need in the list of all packages:

beetsPlugins = lib.recurseIntoAttrs beets.plugins;

@grahamc: While it does solve this particular issue, I'd consider this to be a workaround. What would be a reason to re-expose the plugins other than for Hydra or ofborg eval?

Unrelated to that: What about running nix-instantiate --parse on all .nix files via ofborg? While of course, this wouldn't address full evaluation, but it would address issues like the above.

aszlig added a commit to openlab-aux/vuizvui that referenced this pull request Feb 5, 2021
@aszlig
profiles/base: Disable Beets "Alternatives" plugin
beb276e 
I no longer use this plugin and since evaluation currently is broken
upstream (blocked by [1]), let's make sure that at least the rest of
Vuizvui continues to evaluate.

[1]: NixOS/nixpkgs#109679

Signed-off-by: aszlig <aszlig@nix.build>
@stale
Copy link

stale bot commented Aug 1, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 1, 2021
@mdorman mdorman closed this Jan 13, 2023
@mdorman mdorman deleted the beets-fixup branch January 13, 2023 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 requested changes

Assignees
No one assigned
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mdorman @SuperSandro2000 @grahamc @aszlig