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

lua51Packages.nfd: fix module not loading #309026

Merged
merged 2 commits into from
May 9, 2024
Merged

Conversation

Petingoso
Copy link
Contributor

@Petingoso Petingoso commented May 4, 2024

As #295022 complains, it wasn't finding symbols properly.
This PR fixes #295022 .

I traced it to the migration from buildLuarocksPackage.extraVariables to buildLuarocksPackage.luarocksConfig.variables where someone forgot to add .variables.

It now launches the dialogue properly.

Description of changes

Added a missing property

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.05 Release Notes (or backporting 23.05 and 23.11 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.

As this (issue)[NixOS#295022] complains, it wasn't
finding symbols properly.

I traced it to the (migration)[NixOS#288669] from buildLuarocksPackage.extraVariables to
buildLuarocksPackage.luarocksConfig.variables where someone forgot to
add .variables.

It now launches the dialogue properly.
Copy link
Member

@NotAShelf NotAShelf left a comment

Choose a reason for hiding this comment

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

LGTM!

@teto
Copy link
Member

teto commented May 4, 2024

could you run the tests to prevent further regressions please ? there seems to be busted tests. If that proves too hard, you could at least require the module. There are examples in lua/overrides.nix

@Petingoso
Copy link
Contributor Author

could you run the tests to prevent further regressions please ? there seems to be busted tests. If that proves too hard, you could at least require the module. There are examples in lua/overrides.nix

I can't seem to find any appropriate tests under nixos tests, under the pkgs.lua51packages.nfd.passthru.tests nor in pkgs/tests.

I'm not quite sure about what module you're talking about overriding.

I apologize for my lack of experience with all of these frameworks

@teto
Copy link
Member

teto commented May 4, 2024

sry if that was not clear, my suggestion was to add tests to this nix package because there are none while the source itself seems to have busted tests at least https://github.com/Alloyed/nativefiledialog/tree/master/lua/spec .
You can see how it's done at

checkInputs = [ final.busted ];

(hopefully it will get easier).
No commit in seven years though.

@Petingoso
Copy link
Contributor Author

Petingoso commented May 4, 2024

sry if that was not clear, my suggestion was to add tests to this nix package because there are none while the source itself seems to have busted tests at least https://github.com/Alloyed/nativefiledialog/tree/master/lua/spec . You can see how it's done at

checkInputs = [ final.busted ];

(hopefully it will get easier).
No commit in seven years though.

It actually pulls from a more modern fork, https://github.com/Vexatos/nativefiledialog.
Issue is I'm trying to go through the different phases via a nixshell but being everything wrapped in buildLuaRocksPackage I'm not understanding how to go about it as at the time of CheckPhase i don't seem to have anything built.

Right now when i trigger a CheckPhase it seems luarocks complains Argument missing: please specify a rockspec to use on current directory.

Being the snippet reproducible with just doCheck = true; inside buildLuarocksPackage

Edit: I think it has to do with buildLuarocksPackage running luarocks test on the wrong dir (https://github.com/NixOS/nixpkgs/blob/efd518852d119c174039619a8c84fd11d1afba71/pkgs/development/interpreters/lua-5/build-luarocks-package.nix#L206C1-L206C13)

@Petingoso
Copy link
Contributor Author

After wrestling with this, I think i have tests working

@UlyssesZh
Copy link
Contributor

UlyssesZh commented May 5, 2024

Please add "fix #295022" in the PR comment so that GitHub will automatically close that issue when this PR gets merged. See: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

@UlyssesZh
Copy link
Contributor

For small fixing commits like cba74580b, please squash into previous commits. See commit conventions.

lua51Packages.nfd: cleanup duplicate line

lua51Packages.nfd: cleanup useless undefined hooks

lua51Packages.nfd: squashed commits

lua51Packages.nfd: change to generic buster
@teto teto merged commit e5457f6 into NixOS:master May 9, 2024
23 of 24 checks passed
@teto
Copy link
Member

teto commented May 9, 2024

thank you

@Petingoso Petingoso deleted the nfd_fix branch May 9, 2024 14:07
Petingoso added a commit to Petingoso/nixpkgs that referenced this pull request Jul 22, 2024
This is an attempt to package [Olympus](https://everestapi.github.io/), a GUI for installing Everest and managing Celeste mods.
This is based on this [draft](NixOS#295258), with the authors permission.

It maintains the previous issue of not being able to launch Steam
versions of Celeste from the command line but that's not feasible
without upstream changes.

I only have to note that it has a popup complaining about
finishing the installation but that's due to xdg-mime
x-scheme-handler/everest not being set.

Finally I want to note that it depends on lua51Packages.nfd, which
is broken while [this](NixOS#309026)
isn't merged so currently has the fix there.

Finally, [Lönn](https://github.com/CelestialCartographers/Loenn) works
as expected, being the installation managed by the program.

Ahorn is deprecated(in favor of Loenn) and it crashes over trying to run dynamic
executables

olympus: fixed zenity argument

olympus: bugfix add openssl to fhs

olympus: fixed to use the default nfd instead of patched version

olympus: changed location of ndf arg

olympus: bugfix: add xdg-utils as runtime dependency to fix pop-up

olympus: deleted zenity.patch

Due to nfd lua package being fixed, this patch is no longer needed

olympus: fix use of wrong nfd package

olympus: bumped up version to 24.07.06.02
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.

lua51Packages.nfd is broken
6 participants