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.

Sign up for GitHub

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

minetest: 5.9.1 -> 5.10.0, rename to luanti #355231

Merged
merged 4 commits into from
Dec 24, 2024
Merged

minetest: 5.9.1 -> 5.10.0, rename to luanti #355231

merged 4 commits into from
Dec 24, 2024
+189 −158

Conversation

06kellyjac
Copy link
Member

@06kellyjac 06kellyjac commented Nov 11, 2024 •
edited by fgaz
Loading

Closes #350344

  • minetest: 5.9.1 -> 5.10.0
  • minetest: rename to luanti

https://blog.minetest.net/2024/10/13/Introducing-Our-New-Name/

image
image

Notes:

I removed a substitution for /bin/rm as it had no matches anymore
pagezero_size can probably be removed, I don't see any matches
Moved to pkgs/by-name and usage of darwin.apple_sdk.frameworks.OpenAL for darwin might need tweaking, it doesn't fallback on openal but IDK if it ever needed to
Have aliases in nixpkgs for minetest, added to the descriptiont to also help matches
Program still works with minetest because of symlinks

TODO:

Probably add release notes
Check if it needs backporting to 24.11

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.

@06kellyjac
Copy link
Member Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 355231

x86_64-linux

✅ 3 packages built:
  • luanti
  • luanticlient
  • luantiserver

@ofborg ofborg bot added 8.has: clean-up 8.has: package (new) This PR adds a new package labels Nov 12, 2024
@ofborg ofborg bot requested review from fpletz, PyroLagus and fgaz November 12, 2024 05:11
Mindavi
Mindavi approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@Mindavi Mindavi left a comment

Choose a reason for hiding this comment

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

Diff looks good to me.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 12, 2024
@06kellyjac
Copy link
Member Author

Thanks @Mindavi for the review.

I'd also like some approval from the maintainers. At which point I'll write the release notes and then we can look to merge 🙂

@06kellyjac 06kellyjac added the 8.has: package (update) This PR updates a package to a newer version label Nov 12, 2024
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Nov 12, 2024
@06kellyjac
Copy link
Member Author

I rebased incase this PR was affected by the latest treewide pkgs/by-name migration but it wasn't
I took the opportunity to add a release note.

fgaz
fgaz requested changes Nov 12, 2024
View reviewed changes
Copy link
Member

@fgaz fgaz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks!

This is a breaking change, so let's wait for the 24.11 branch-off before merging.

While we wait, there is a minetest module that I think we should rename as well.

Also could you please reformat the file in a separate commit so the diff is easier to review?

pkgs/by-name/lu/luanti/package.nix Outdated Show resolved Hide resolved
@fgaz fgaz added the 2.status: wait for branch‐off Waiting for the next Nixpkgs branch‐off label Nov 12, 2024
@06kellyjac
Copy link
Member Author

Ah, yesterday luanti.org was still just memes. I'll update that now.

In reality there isn't much in the way of breaking but I get what you mean. We're still good to backport into the 24.11 branch prior to release, just once it's branched off?

The move was in a separate commit to the format, or are you looking for 3 commits: format, update, rename?

@fpletz
Copy link
Member

fpletz commented Nov 12, 2024

This is a breaking change, so let's wait for the 24.11 branch-off before merging.

Since the aliases are in place I don't think we need to wait for the branch-off. I think we should backport the name change anyway. It's a pity this is hitting right during our release freeze but this doesn't touch anything else anyway, right?

@fgaz
Copy link
Member

fgaz commented Nov 12, 2024

are you looking for 3 commits: format, update, rename?

Yes, please

@06kellyjac 06kellyjac force-pushed the minetest_luanti branch 2 times, most recently from b3a7138 to bc1b028 Compare November 12, 2024 14:38
@06kellyjac
Copy link
Member Author

ok, commits as they should be. New url. I also updated the description to match their website & how they frame it more as a voxel game engine rather than just a game.

I'm also interested in helping maintenance for luanti/minetest if you'll have me. If so should I add it as a separate commit? 🙂

@fgaz
Copy link
Member

fgaz commented Nov 12, 2024 •
edited
Loading

Regarding compatibility, the rename of bin/minetest in particular is my biggest concern, but isn't the only breaking change: https://dev.minetest.net/Changelog#5.9.1_.E2.86.92_5.10.0

So personally I wouldn't backport it either, but I don't feel strongly about it if the other maintainers disagree.

edit: I didn't notice the minetest -> luanti symlinks!

I'm also interested in helping maintenance for luanti/minetest if you'll have me. If so should I add it as a separate commit? 🙂

Feel free to :)

@fpletz
Copy link
Member

fpletz commented Nov 12, 2024

Mentioning @RossComputerGuy and @wegank as 24.11 release managers for their input on the breaking change involved here.

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 12, 2024
@ofborg ofborg bot requested a review from fgaz November 13, 2024 01:54
@06kellyjac 06kellyjac force-pushed the minetest_luanti branch 2 times, most recently from 999f70d to 6249dc5 Compare November 13, 2024 11:12
@ofborg ofborg bot added the ofborg-internal-error Ofborg encountered an error label Nov 13, 2024
@RossComputerGuy
Copy link
Member

Result of nixpkgs-review pr 355231 run on aarch64-linux 1

1 package blacklisted:
  • nixos-install-tools
3 packages built:
  • luanti
  • luanticlient
  • luantiserver

RossComputerGuy
RossComputerGuy reviewed Nov 14, 2024
View reviewed changes
pkgs/top-level/aliases.nix Outdated
Comment on lines 771 to 836
minetest = luanti; # Added 2024-11-11
minetestclient = luanticlient; # Added 2024-11-11
minetestserver = luantiserver; # Added 2024-11-11
minetest-touch = luanticlient; # Added 2024-08-12
minetestclient_5 = luanticlient; # Added 2023-12-11
minetestserver_5 = luantiserver; # Added 2023-12-11
Copy link
Member

Choose a reason for hiding this comment

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

I think these should be replaced with throws. Take a look at mod_dnssd which is a few lines down for an example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I'm tweaking them to throws and fixing the formatting complaint

Any thoughts from people on dropping the _5 items completely vs adding a luanticlient_5 alias?
Also what to do with minetest-touch.
And maybe while we're renaming stuff, minetestclient -> luanti-client would be nice IMO

cc @fpletz @fgaz

Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts from people on dropping the _5 items completely vs adding a luanticlient_5 alias? Also what to do with minetest-touch.

I'd drop both

And maybe while we're renaming stuff, minetestclient -> luanti-client would be nice IMO

On one hand the server executable is named minetestserver so it makes sense to follow the same pattern. On the other, all other distro packages are dashed.

I agree with renaming.

Copy link
Member

Choose a reason for hiding this comment

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

I dropped the _5 aliases completely, but I did not make the others throw since the module still uses pkgs.minetest

@fpletz fpletz removed the 2.status: wait for branch‐off Waiting for the next Nixpkgs branch‐off label Nov 29, 2024
@nyabinary
Copy link
Contributor

Any status on this :p

@fgaz
Copy link
Member

fgaz commented Dec 13, 2024

@06kellyjac are you still planning to finish this? let us know if you want us to take the PR to the finish line instead

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Dec 21, 2024
@nyabinary
Copy link
Contributor

@06kellyjac are you still planning to finish this? let us know if you want us to take the PR to the finish line instead

No response, So I think you should prob supersede this atp?

@06kellyjac @fgaz
minetest: format
5fef87a 
Format to match the new standards in nixpkgs
@fgaz fgaz force-pushed the minetest_luanti branch 2 times, most recently from 816b828 to a878e85 Compare December 24, 2024 15:38
@06kellyjac @fgaz
minetest: rename to luanti
95c217a 
Moved to pkgs/by-name
Added and updated aliases
Added release note
@06kellyjac @fgaz
luanti: add jk to maintainers list
3e67eb7
fgaz
fgaz approved these changes Dec 24, 2024
View reviewed changes
Copy link
Member

@fgaz fgaz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks everyone!

I fixed the conflict, made the changes we agreed on, moved the changelog entry to 25.05, and moved the rm substitution to the patch itself.

I'll merge in a bit.

@06kellyjac
Copy link
Member Author

Sorry, been busy over Christmas. Thanks @fgaz

@fgaz fgaz merged commit a70c3d1 into NixOS:master Dec 24, 2024
24 of 25 checks passed
@06kellyjac 06kellyjac deleted the minetest_luanti branch December 25, 2024 13:38
@cole-h cole-h removed the ofborg-internal-error Ofborg encountered an error label Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RossComputerGuy RossComputerGuy RossComputerGuy left review comments

@Mindavi Mindavi Mindavi approved these changes

@fgaz fgaz fgaz approved these changes

@bachp bachp bachp approved these changes

@fpletz fpletz Awaiting requested review from fpletz

@PyroLagus PyroLagus Awaiting requested review from PyroLagus

Assignees
No one assigned
Labels
6.topic: games 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: clean-up 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 8.has: package (update) This PR updates a package to a newer version 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

minetest: Minetest has begun the process of changing to Luanti
9 participants
@06kellyjac @fpletz @fgaz @RossComputerGuy @nyabinary @bachp @Mindavi @wegank @cole-h