dizzyd / mcdex

Minecraft Modpack Management
Apache License 2.0
75 stars 8 forks source link

Add command for removing a mod from a pack #37

Open mgziminsky opened 5 years ago

mgziminsky commented 5 years ago

For this, I implemented both a basic, single mod version, and a recursive version that also removes any mods that depend on the one being removed. The recursive version is "disabled" though, since I couldn't find any way to distinguish between required and optional dependencies.

I'm not sure how you generate your database, but would it be possible to add that information into the moddeps table? If it's possible, I'd be willing to do it if you directed me to the code.

mgziminsky commented 5 years ago

Ok, so looking at commit https://github.com/dizzyd/mcdex/commit/1f8d0ef03add8a443e8d4708924bcda368367eb7, it seems level is the dependency type, I had interpreted it as a sort of hierarchy depth. I thought I was filtering to 1, but I just realized that was only in my test query.

I can see that 1 == Required, I assume 2 == Optional, what is 3?

Thanks.

dizzyd commented 5 years ago

Hmm, maybe I missed it, but I'm not seeing any code that ensures you're not inadvertently removing a dependency that another mod requires? I.e. If mod X and Y depend on mod A, and the user removes mod X, mod A will also get removed, thus breaking Y.

mgziminsky commented 5 years ago

Hold off on reviewing this too deeply for now since I'm overhauling now that I know what level is. Regarding your question, it's actually the other way around right now, although I should probably have it remove dependencies too. The enabled, single mod version doesn't do any dependency checks. The recursive version only looks at dependents, not dependencies as of now. So in your example, removing A would also remove both X and Y, and anything that depends on X or Y etc, but removing X or Y would leave A.

I actually spent a good deal of time yesterday reworking the recursive code since I figured out what level was, and I'll probably unify them to a certain extent for at least reporting purposes if nothing else. Not removing dependencies was just an oversight by me, I just didn't think of that direction for some reason. It shouldn't be too difficult to handle though.

Also, FYI, while working yesterday, I noticed that the new deps table is completely unoptimized, and querying it kills performance. It is full of duplicates and has no types or indices. I recreated it using the following queries setting the types, PK, and some indices, and my recursive query went from 30 sec to instant. My guess is the duplicates are coming from multiple MC versions, but there are no cases of them having different levels.

CREATE TABLE deps_dg_tmp
(
  fileid    INTEGER,
  projectid INTEGER,
  level     INTEGER,
  CONSTRAINT deps_pk PRIMARY KEY (fileid, projectid)
);
INSERT INTO deps_dg_tmp(fileid, projectid, level) SELECT DISTINCT fileid,projectid,level FROM deps;
DROP TABLE deps;
ALTER TABLE deps_dg_tmp RENAME TO deps;
CREATE INDEX deps_fileid_index ON deps (fileid);
CREATE INDEX deps_projectid_index ON deps (projectid);
dizzyd commented 5 years ago

Thanks for pointing this out about the deps table. I'll tweak the crawler to use a primary key on deps and rebuild everything.

dizzyd commented 5 years ago

@mgziminsky - I've updated the crawler to have a primary key on deps table; let me know if that addresses your performance issues, please? The latest db available on S3 should reflect this change.

mgziminsky commented 5 years ago

Thanks, that did the trick. I'm in the process of putting the finishing touches on my code and should have this updated soon.

Edit: Removed missing db entries remark and log output to remove clutter from the PR. Issue was resolved via discussion in discord.

dizzyd commented 5 years ago

@mgziminsky - could you ping me on the Forge discord or some other system with IM? I'm dizzyd#7825 on Discord or @dizzyd on Twitter.