-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
rimsort: init at 1.0.6.2-hf-unstable-2024-04-26 #304943
Conversation
9826dc0
to
e15e882
Compare
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.
Commit history looks good, so far everything looks good for the most part aside from a few nitpicks. You probably should update your top comment on this PR to say Closes #304943
so GitHub links this PR to the related issue.
pkgs/by-name/ri/rimsort/package.nix
Outdated
}: | ||
python3Packages.buildPythonApplication rec { | ||
pname = "rimsort"; | ||
version = "unstable-2024-04-15"; |
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.
Probably should prefix the version with 0-
.
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.
RimSort has tags in a suitable format, so 0
is wrong. 1.0.6.2-hf-unstable-<et cetera>
should be used. Checked by letting unstableGitUpdater
do a bump via #280501.
pkgs/by-name/ri/rimsort/package.nix
Outdated
xmltodict | ||
]; | ||
|
||
propagatedBuildInputs = [] ++ python-deps; |
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.
Probably don't need the empty list.
pkgs/by-name/ri/rimsort/package.nix
Outdated
patches = [ | ||
./remove-name-main-conditions.patch | ||
./use-writeable-data-folder.patch | ||
]; |
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.
It is common to have patches
towards the top, below the src
.
}: | ||
buildPythonPackage rec { | ||
pname = "steamfiles"; | ||
version = "unstable-2020-10-26"; |
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.
Probably should also prefix the version here with 0-
.
}: | ||
buildPythonPackage rec { | ||
pname = "steamworks"; | ||
version = "unstable-2023-11-25"; |
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.
Same here with the prefixing of the version.
}: | ||
buildPythonPackage rec { | ||
pname = "win-inet-pton"; | ||
version = "unstable-2019-02-19"; |
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.
Same here with the version prefixing.
17558ea
to
15451c7
Compare
From the current state: Result of 22 packages built:
|
Trying to run and download a workshop mod from steam through the app causes this (using the Steam app and not SteamCMD)
|
Hi @nyabinary , RimSort dev here Could you please file an Issue on the RimSort repo and I will look into fixing this evening? |
15451c7
to
9aa7e9f
Compare
Downloaded the db and rules manually and tried to download a workshop mod, and it's just infinitely loading (this is via the Steam App)
|
66c5ac8
to
994bfb1
Compare
@nyabinary thanks, that + your last comment in the rimsort repo are both things for me to look into |
8cfe341
to
05a4a30
Compare
1bc2240
to
63cb445
Compare
ok as hacky as some of it is i think everything is working for me now. will have to wait for twsta i guess to see if not running rimsort from steam is cause of some of the other issues |
|
not sure how I feel about making it a module primarily because even though some user's indeed won't need to use steamcmd, it doesn't align with how the real project is packaged - since on pretty much any non-nix distro everything builtin to rimsort to download steamcmd and such will just work, it'd be expected that it just works im not entirely opposed to it though. i just dont like it. im trying to find any official guidance on when I might want to actually make something a module vs not. a middle ground might be just making some features overrideable? e.g. i still don't love that specifically though because:
edit: but in this case i still dont like the override because the design of rimsort sort of assumes everything either is there or can just be downloaded on the fly, i.e. id be happier with that option if rimsort itself knew that it shouldn't show steamcmd options or a download button for it |
You should join the Rimsort Discord since it would be easier to communicate there than clogging this github thread lol |
RimSort.log |
d993bff
to
752bf4f
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/3824 |
|
||
nativeBuildInputs = [ | ||
setuptools | ||
wheel |
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.
wheel is probably not required here
|
||
nativeBuildInputs = [ | ||
setuptools | ||
wheel |
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.
wheel |
]; | ||
|
||
propagatedBuildInputs = [ | ||
coverage |
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.
We don't want to collect coverage as it slows down the tests and we have no use for it
coverage | ||
gevent | ||
pytest | ||
pytest-cov |
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.
pytest-cov |
|
||
nativeBuildInputs = [ | ||
setuptools | ||
wheel |
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.
same here
protobuf | ||
protobuf3-to-dict | ||
]; | ||
|
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.
Can we execute the tests https://github.com/twstagg/steamfiles/tree/master/tests ?
@@ -34,4 +34,4 @@ setup( | ||
install_requires = [], | ||
extras_require = {}, | ||
zip_safe = True | ||
-) | ||
\ No newline at end of file | ||
+) | ||
|
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.
@@ -34,4 +34,4 @@ setup( | |
install_requires = [], | |
extras_require = {}, | |
zip_safe = True | |
-) | |
\ No newline at end of file | |
+) |
pkgs/by-name/to/todds/package.nix
Outdated
repo = "todds"; | ||
rev = version; | ||
hash = "sha256-nyYFYym9ZZskkaTPV30+QavdqpvVopnIXXZC6zkeu7c="; | ||
fetchSubmodules = true; |
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.
Can we use the vcpkg from nixpkgs instead?
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.
turns out it wasn't needed at all :p didn't think to check if the submodule was actually necessary for the build
}; | ||
|
||
patches = [ | ||
./make-nix-compatible.patch |
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.
Can you try to upstream the generic fixes in there?
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.
@SuperSandro2000 this was my big question, don't know what fixes would be accepted or on what timeline etc.
i can try, i dont mind this pr sitting here people can still use it anyways, i just wasn't sure if it'd be preferable to get it working with ugly patches, get it merged, eventually get stuff upstreamed, then remove patches when the package gets updated, or to try to upstream 1st before ever merging
in either case i am going to try to upstream what i can
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.
Any progress on that?
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.
@nyabinary no. the rimsort guy didn't reply to my messages about this specifically and seemingly deleted another nix-related thread.
i redid my patches in a way that is much cleaner but it also turned out to be significantly more lines changed to make it pretty
i'm just going to close this pr for now because i realize im not interested in maintaining rimsort as it is now. i'll leave my branch alive on my fork in case either someone else wants to pick it up or if eventually the generic changes can be upstreamed
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.
Hello. I am the guy you speak of. I am not sure what you are referring to that i am supposed to have deleted or not responded to.
I am open to supporting NixOS upstream, but am only willing to support code that we work on upstream. I am always open to discussing further, however limited my direct involvement in implementing may be.
I am not really trying to provide support for these patches specifically, and i think that some of what i have looked at warrants discussion. Some of those comments and observations i initially had probably got lost on the discord.
752bf4f
to
0bb8f77
Compare
0bb8f77
to
5987030
Compare
5987030
to
1fad08c
Compare
1fad08c
to
6e5179b
Compare
Hey @vinnymeller, are you still interested in maintaining RimSort? |
Prob not so if you want to take over that would be nice :P |
Prob not so if you want to take over that would be nice :P
I've updated RimSort to the latest commit, and updated the patch. Unfortunately, it looks like more changes are necessary to make sure the SteamCMD wrapper correctly detects our own steamcmd binary.
|
@SigmaSquadron sorry I was on vacation when you first tagged me and I guess I missed it I definitely don't want to maintain this as overall it's just not super... nix friendly? You're definitely right though before I gave up I was in the middle of debugging that issue but sadly that branch was lost between then and now. If you try to fix it, good luck 🙂 |
Description of changes
Closes #304832
Adds RimSort
Seemingly everything works now
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-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.