-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
amulet-map-editor: init at 0.10.44 #405548
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
base: master
Are you sure you want to change the base?
Conversation
Sigmanificient
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, welcome to nixpkgs and thanks you for your awesome work!
Theses two change request apply to all the derivation in order to follow current buildPython{Application,Package} standards, sorry this is quite some changes to do.
Lets also make sure we can importa theses packages using pyrhonImportsCheck!
|
Hey :) I'm okay with closing my PR in favor of yours if I could be credited as a co-author on changes that were taken from my PR (add |
d89c827 to
6c2ac08
Compare
|
I've fixed all mentioned issues. Edit: I will try to remove the overrides. |
6c2ac08 to
88c46c1
Compare
|
I've fixed mentioned issues again, minus the modules tests because they fail for reasons I don't understand. |
934f9e3 to
fea7b8e
Compare
alyssais
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack license.
fea7b8e to
90a4a62
Compare
|
Okay now we have the tests running and passing except on I asked for help on our Discord server to add the tests. @langsjo discovered that the reason I was having issues was because the modules I fetched from PyPI didn’t include any tests. The solution was to use I sincerely hope this PR is ready to go now. |
|
Uhm, the license that you set has not been merged into their |
|
If I read that PR correctly, there was no licence in the repository at all previously, and they have just pinged every past contributor saying they’re going to assume their changes were under MIT without there having previously been any indication of that? That is, uh, not how it works. If I understand correctly, then this is going to have to be |
|
As I understand it the core maintainer(s) weren't really concerned about the legal implications of unclear licenses for us packagers. It feels like an unspoken agreement that the repositories should be in their custom Polyform Shield fork, and they never bothered to codify it as such. I personally wouldn't take any chances and would only label the packages with an explicit LICENSE file with |
I guess it really is time for
I think they also all signed a CLA. Not a lawyer but judging from its wording they are able to just change licenses to their heart's content once a contributor has signed the CLA:
Longer clauseSubject to the terms and conditions of this Agreement, You hereby grant to Us a worldwide, royalty-free, NON-exclusive, perpetual and irrevocable (except as stated in Section 8.2) license, with the right to transfer an unlimited number of non-exclusive licenses or to grant sublicenses to third parties, under the Copyright covering the Contribution to use the Contribution by all means, including, but not limited to: Looks like they started using If you want to see for yourself, I got this info from the github insights page |
Oh, we’ve been there for a long time…
These are clearly substantial enough contributions to be copyrightable. If they don’t have permission from these contributors then we can’t record this as Polyform regardless of what the repository says. |
|
Okay, so if I understand the licensing issue correctly, the Amulet license is somehow flawed. I looked at #422452 and there seem to be other issues I haven't encountered myself that would need fixing, but I could work with @erkingoksuu on a following PR to add his fixes. I'll update this PR for version 0.10.44 whenever I have a bit of time. I'd really like to be done with this PR as it adds me in the maintainer list and there are other PRs that I would like to do. =) |
|
I think you'd need to carefully look which project do and don't include a LICENSE file with the amulet license in it. If it doesn't it should be changed to For example,
I wouldn't do that. Because that PR was closed for a reason. I have no idea if those changes that were deemed as "required" by the LLM are actually needed. It would be much better if a human looked at it instead... Happy to help though if you need it (I would need some leads on what's wrong with the package as of now but I think that goes without saying). |
|
Sorry for this being such a pain. I am not a legal expert and licenses make my head hurt.
Correct. Newer contributions are not an issue.
The advice from my boss (and sponsor of the project) was that contributions to a repository without a license inherit the default license which may be a misunderstanding of section D6 of the terms of service. I think until we can sort out the license issues your simplest solution is to list it under It is also worth noting that I have been working on a large rewrite of a lot of these libraries so the default branch may not be the version you are using and may contain a license where the old one does not. |
90a4a62 to
677c185
Compare
|
Okay I updated the editor and the modules and changed the metadatas to use I retested the editor and it still works with the UI issues already mentioned before. There are also random Pango segfaults that I forgot to mention before. But its apparently another issue that users experience (Amulet-Team/Amulet-Map-Editor#1012). |
|
I think there is no point in defining |
Co-authored-by: Leah Amelia Chen <[email protected]> Co-authored-by: langsjo <[email protected]>
Co-authored-by: Leah Amelia Chen <[email protected]>
Co-authored-by: Leah Amelia Chen <[email protected]>
Co-authored-by: Leah Amelia Chen <[email protected]>
Co-authored-by: Leah Amelia Chen <[email protected]>
Co-authored-by: Leah Amelia Chen <[email protected]>
677c185 to
8f4580d
Compare
|
I made the change. Should be good for a merge now ? =) |
This PR adds the Amulet Map Editor package to Nixpkgs.
This application is a map editor for the video game Minecraft.
The application has
python-modulesdependencies that I also added individually, in case people want to use them for some Minecraft Python projects.This closes #201016
And I suppose also closes #356035
Some peeps on Discord told me there was already a PR for this package, but silly me must have forgotten to check for a previous PR. So I've quickly added some stuff from the other PR (thank you @pluiedev) but I did not include stuff like tests and Sphinx docs stuff because I haven't looked into those yet, so I don't know how those things work.
Tell me if I need to add those for this PR.
The application works and is usable, although there are some UI rendering issues related to
wxpython, which make the application hard to use. The application devs are aware of the UI issues (seems to be related to this issue #127) and they seem to occur for any linux user (so also for other distros).The devs are working on replacing
wxpythonto fix the UI issues.Things done
maintainers.listamulettolicenses.listamulet-map-editorapplicationThe following
python-moduleswere also added:Building and testing:
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.