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

mudlet: 3.0.0-delta -> 4.0.3 (fixes broken package) #68158

Merged
merged 3 commits into from Sep 7, 2019
Merged

Conversation

pstn
Copy link
Contributor

@pstn pstn commented Sep 5, 2019

Motivation for this change

mudlet has been broken for a while now. I managed to fix and upgrade it to the most recent version.

I added myself as a maintainer since nobody else seems to have interest in that package any more.

Thanks to @abbradar for the help and the patch to upstream.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @Wyvie

@ofborg ofborg bot added the 6.topic: lua label Sep 5, 2019
@pstn pstn changed the title Mudlet mudlet: 3.0.0-delta -> 4.0.3 (fixes broken package) Sep 5, 2019
@pstn
Copy link
Contributor Author

pstn commented Sep 5, 2019

The current maintainer has been inactive for quite some time, so please consider reviewing this even if you are not them.


makeQtWrapper $out/mudlet $out/bin/mudlet \
--set LUA_CPATH "${luaFileSystemPath};${luaZipPath};${lrexlibPath};${luasqlitePath};${luayajlPath};${luaUtf8Path}" \
--prefix LUA_PATH : "$NIX_LUA_PATH" \
Copy link
Member

Choose a reason for hiding this comment

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

have you tried using an environment instead? luaEnv = lua.withPackages(p: [luaFileSystemPath ...]). Will make the path simpler and indenpendant of the lua version, like one could override with lua52 and still have the paths valid instead of luaUtf8Path = "${luautf8}/lib/lua/5.1/?.so";

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 didn't because I didn't know about that method. You are right. It's way cleaner.
I couldn't get rid of all dependencies on the lua version, but I moved that version in a variable at the top for easier maintenance and discovery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scratch that. Got it working by overwriting the lua version in all-packages.nix.

@pstn pstn force-pushed the mudlet branch 2 times, most recently from 7e75918 to 807f240 Compare September 6, 2019 09:34
@pstn
Copy link
Contributor Author

pstn commented Sep 6, 2019

Removed a few dependencies that became unneeded but I forgot to clean up.

WITH_FONTS = false;
WITH_UPDATER = "NO";

preConfigure = ''
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this is not needed now that we use withPackages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

preConfigure removed.

];

preConfigure = "cd src";
WITH_FONTS = false;
Copy link
Member

Choose a reason for hiding this comment

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

Use "NO" here too for consistency and because I'm not sure false would work but "NO" is used by AUR package.

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.

@pstn pstn force-pushed the mudlet branch 2 times, most recently from 1774d6d to 20702b1 Compare September 6, 2019 11:51
Copy link
Member

@abbradar abbradar left a comment

Choose a reason for hiding this comment

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

@teto, can you check Lua stuff once more just in case before we merge?

nativeBuildInputs = [ makeWrapper qmake ];
patches = [
( fetchpatch {
url = "https://github.com/abbradar/Mudlet/commit/4f9450e9f66ac72f502e52951ff04b8ac8d123b3.patch";
Copy link
Member

Choose a reason for hiding this comment

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

may be best for the patch to live in nixpkgs ?

Copy link
Member

Choose a reason for hiding this comment

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

They already merged it upstream actually: Mudlet/Mudlet@6306f99

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 had it in nixpkgs but @abbradar wanted me to pull it from their repo.

I think i like it better this way because you could access more information about the patch when you know how to handle the link.

Copy link
Member

Choose a reason for hiding this comment

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

the issue is that url points towards @abbradar url. so if he deletes his fork or the branch, the package will stop compiling. If the patch were in nixpkgs then we are in control. Referencing the upstreaming patch should be fine as if their commit disappear, it means we will have problems anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, got it. fetching the patch from mudlet/mudlet now.

Copy link
Member

@teto teto left a comment

Choose a reason for hiding this comment

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

there are too many changes in the lua generated packages list. I wonder if some of them are linked with the luarocks update.
Many seem to have changed to luafr.org, which is a mirror of the luarocks server. Are you based in france by any chance @pstn ? This is something I need to fix in luarocks2nix Meanwhile you could just restrict the changes in in generated-packages.nix to only the changes you need (with git add -p).

Philipp added 2 commits September 7, 2019 12:23
Dependency for mudlet >= 4.0
Dependency for mudlet >= 4.0
@pstn
Copy link
Contributor Author

pstn commented Sep 7, 2019

No, I'm in Germany.

I did discard all the unneeded changes in both commits. Thanks for your help.

@teto
Copy link
Member

teto commented Sep 7, 2019

No, it's me thanking you ! I wanted this package to use lua environments for at least a year but fixing it proved too difficult. Not only did you do it but you also found some issue in how the lua packages are generated. I have a fix I will submit soon.
The last thing I would like to see before merging is to fetch the patch from the mudlet's repo. Thanks for keeping up with us.

Fixed broken package.
@teto
Copy link
Member

teto commented Sep 7, 2019

@GrahamcOfBorg build luaPackages.mudlet

@teto teto mentioned this pull request Sep 7, 2019
10 tasks
@teto
Copy link
Member

teto commented Sep 7, 2019

not sure why it's stuck, I ran a nix-review on x86-64 and it worked. Merging. Thanks to all those involved.

@teto teto merged commit 3b5b9a7 into NixOS:master Sep 7, 2019
@pstn pstn deleted the mudlet branch September 12, 2019 14:42
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.

None yet

3 participants