0neGal / viper

Launcher+Updater for TF|2 Northstar
https://0negal.github.io/viper
GNU General Public License v3.0
151 stars 21 forks source link

bug: `manifest.json` does not uniquely identify a Thunderstore mod #165

Closed GeckoEidechse closed 1 year ago

GeckoEidechse commented 1 year ago

This is a bug that might not be one yet but will become an issue in the future. Basically, the Thunderstore manifest.json does not uniquely identify a mod as it lacks the author_name of a Thunderstore mod (as well as the namespace but that's not an issue in our case).

This means if two mod authors both upload a mod with the same Thunderstore mod name, Viper will be unable to tell which author a Northstar mod belongs to.

I was in the process of updating FlightCore code base from writing Thunderstore mod string to mod.json to just putting the Thunderstore manifest.json into the mod folder like Viper does when I came across this.

As such between Viper, VTOL, and FlightCore, we probably wanna implement a unified solution that uniquely identifies a Thunderstore mod and is used by all 3 of us to ensure compatibility between mod managers ^^

EDIT:

For example, looking only at manifest.json you cannot differentiate the two mods when ignoring changeable fields (version_number, website_url, ....)

image

image

GeckoEidechse commented 1 year ago

@BigSpice for VTOL

GeckoEidechse commented 1 year ago

Potential solutions would include putting the author name into manifest.json ourselves post-install, adding the entire Thunderstore mod string to it, adding a separate file with the mod string, or something else entirely.

Thoughts?

GeckoEidechse commented 1 year ago

@AnActualEmerald for libthermite

AnActualEmerald commented 1 year ago

I don't love the idea of editing extracted files post-install, I'd rather add a new file with the mod string, perhaps called something like .mod_id? Wouldn't even need to be JSON formatted or anything

GeckoEidechse commented 1 year ago

Yeah, not editing any existing files causes the least issues. Though I'd change the filename so it's not hidden (on *nix) by default, so no leading dot and maybe something that is rather verbose, so that any random player checking their mod files can guess what it does, so like for example thunderstore_mod_id?

Maybe also with .txt file ending (thunderstore_mod_id.txt) so that they can easily inspect it and figure out what it does. This does however come at the risk of being more likely to be (accidentally) edited by some user when messing around with their files.

The other question is, do we wanna put the whole modstring in there, i.e. Fifty-Server_Utilities-2.2.1 or only author and modname and grab version from manifest.json?

ASpoonPlaysGames commented 1 year ago

files downloaded from thunderstore are called packages, so thunderstore_package_id.txt would be better, might as well put the whole dependency string in there, else it would just be author.txt really

0neGal commented 1 year ago

I think the general idea should be, to not attempt to populate manifest.json with anything (if your launcher/manager even creates it that is). Not only does it overcomplicate things in terms of the code and handling it, but it also means we all would have to agree to format the JSON in a way we all can read without any problems.

I think, also as stated above, a simple plaintext file with the author name, is all that's needed. And then it's up to the individual launcher/manager to use it, when distinguishing packages.

Something as simple as thunderstore_author.txt, or more generalized mod_author.txt/package_author.txt would work best or perhaps even author.txt as suggested before, making it more relevant for dependencies/libraries. And the should just simply contain the author name and nothing more.

In reality, this is sort of just an issue created because of Thunderstore not deciding to include the author inside the manifest file (although I get why that's not the case), and Northstar not requiring an author value either. But I digress.

BigSpice commented 1 year ago

I still think the best way about this is to have the mod.json change. Since we have direct and easy control to that.

In the case of the author's really needing iden, wouldn't adding the author's name to the mod.json once you unpack the mod.

@GeckoEidechse Northstar does not break with extra Val's in the JSON right?.

BigSpice commented 1 year ago

Mandatory author name's in the mod.json would be the fastest imo.

If it comes down to it, The way I would do this in VTOL would be to essentially have the mod author be under a custom JSON we all follow, Which dumps the name, author name, links, etc for our use. Any mm can just call that file for relevant info on the mod.

A mod_manifest?,

Probably dump it next to the mod.json. Would be even better since we could use that file for enabling and disabling too.

0neGal commented 1 year ago

While I like the idea of the author being strictly in the mod.json and being official, it really wouldn't be the best, every guide that already exists to make Northstar mods would need to be updated, and every mod not updated won't have an author. So it is actually, from my viewpoint the slowest solution.

On top of this, it'd be user editable, unlike with getting the Thunderstore author. So any mod author could put anything in there, not ideal.

We have basically all the data we'd ever need in the manifest.json and mod.json files, except the author, so making an entirely new file doesn't seem like that great of a solution, just to make everything be in one file. Besides that, minimizing the amount of things to be unofficially "standardized" is key. So making these unofficial standards as simple as possible is inherently key as well.

Therefore, I still think a simple plaintext file like author.txt (file name is debatable), to be the best solution.

GeckoEidechse commented 1 year ago

[...] and Northstar not requiring an author value either. But I digress.

That would not help anyway, cause we need Thunderstore author string here, not Northstar mod author (they can differ) :P

@GeckoEidechse Northstar does not break with extra Val's in the JSON right?.

Northstar handles it just fine ye, been doing it for a while without issue but I wanna switch away cause just like @AnActualEmerald, I don't love the idea of editing extracted files post-install.

Therefore, I still think a simple plaintext file like author.txt (file name is debatable), to be the best solution.

I'm kinda tending towards thunderstore_mod_id.txt right now which contains the full modstring, e.g.Fifty-Server_Utilities-2.2.1, simply cause extracting mod author name to write to file requires slightly more logic and with the full mod string we could use it as extra parity to check against manifest.json.

If we do go for "just author", I'd say the file should be named thunderstore_mod_author.txt or similar. Basically it should contain thunderstore to indicate that "author" is the Thunderstore author (not the Northstar mod one).

BigSpice commented 1 year ago

I feel 1 file which solves all of this would be ideal.

That way future issues are resolved by this as well, it's best Imo that we establish a long-term solution. Something that should we need, as a collective, to add some info, it's a painless process. Maybe just having the TXT or JSON be a generic name would work.

0neGal commented 1 year ago

[...] and Northstar not requiring an author value either. But I digress.

That would not help anyway, cause we need Thunderstore author string here, not Northstar mod author (they can differ) :P

I did forget to mention here, that I don't actually think it would solve the problem, as it would allow anybody to edit the file and add a name that isn't their own.

I'm kinda tending towards thunderstore_mod_id.txt right now which contains the full modstring, e.g.Fifty-Server_Utilities-2.2.1, simply cause extracting mod author name to write to file requires slightly more logic and with the full mod string we could use it as extra parity to check against manifest.json.

It really does not require more work, owner is part of the package's object in the Thunderstore API, which is equivalent to the author. I also think it being a sort of parity mechanism would overcomplicate it as well, having to make checks for all of that.

In my opinion thunderstore_author.txt seems most ideal with all this in mind.

I feel 1 file which solves all of this would be ideal.

That way future issues are resolved by this as well, it's best Imo that we establish a long-term solution. Something that should we need, as a collective, to add some info, it's a painless process. Maybe just having the TXT or JSON be a generic name would work.

While I get the sentiment with having a file with more information than just author name, and making it possible to easily extend it. It could cause issues, it's not as likely, but interoperability especially between different versions of various launchers/managers could be hard to ensure.

But in case we do go with a slightly more extensible format, we could simply have a file like thunderstore_misc.json/thunderstore.json, and inside it, to begin with just have:

{
    "author": ""
}

The major issue here, is if we start adding many things in here it could overcomplicate the file, more importantly, if in the future something is changed, like a key is renamed or similar, we would potentially break backwards compatibility and interoperability.

Compared to having a simple thunderstore_author.txt file, adding checks for if it exists, and if it does actually do something with it. Removing the file in the future would break nothing for either future or older versions.

And so the problem here is really just interoperability and backwards compatibility, if each launcher/manager used it's own file and format, they, if responsible, would work in backwards compatibility of some kind. However if we have a shared way of doing things, and we make a change that requires updated code to support backwards compatibility and or not cause problems, we'll inevitably have someone who isn't compliant or isn't so instantly.

But that's just my view on it, at least...

GeckoEidechse commented 1 year ago

While I get the sentiment with having a file with more information than just author name, and making it possible to easily extend it. It could cause issues, it's not as likely, but interoperability especially between different versions of various launchers/managers could be hard to ensure.

In fact I think if we would decide to one day go from thunderstore_author.txt to thunderstore_misc.json we would face the exact same issues. Or to rephrase it, I don't think we'll need to define more than the missing author and as such can just stick with the single text file with author name solution. Should the need for more arise, we'll face similar issues as if we'd gone for the JSON file that stores more information in the first and so as such should just stick to the simpler solution for now ^^

 

That being said, for FlightCore I just use @AnActualEmerald's library and don't really implement much myself, so I'm fine with leaving the final decision of whether to do single thunderstore_author.txt or a full thunderstore_misc.json to the 3 of you (@0neGal @AnActualEmerald @BigSpice) :P

0neGal commented 1 year ago

In fact I think if we would decide to one day go from thunderstore_author.txt to thunderstore_misc.json we would face the exact same issues.

Not entirely. In the case of a .txt file, if we switch over to a new format, we just remove code for handling the file, and make the new one, and older versions will still function as they should of course have handling for if the file doesn't exist to begin with, obviously.

BigSpice commented 1 year ago

JSON would be nice for parsing reasons. But if the aim is speed and reliability, then TXT is the way to go. as of this time, I have no implemented use for the file as of yet, but I will generate it in accordance.

0neGal commented 1 year ago

JSON would be nice for parsing reasons. But if the aim is speed and reliability, then TXT is the way to go.

I would argue it'd be far easier, simply read the file, and maybe remove leading and trailing whitespace, and or only read the first line. Compared to parsing JSON and hoping it's not formatted horribly for whatever reason.

BigSpice commented 1 year ago

But JSON formatting is easy, and you just have to read for the var you want?. I'm pretty sure all of is here can generate a JSON file just fine though....

BigSpice commented 1 year ago

Whatever the format is, I'm good with it, just wanted to say, if we are looking at shared data now. Could we discuss the uri link handling?.

BigSpice commented 1 year ago

I was thinking that if 1 manager wanted to become the main handler, should we check for other installs of other managers?.(if viper wanted to handle links but r2mm existed, r2mm would be default. Etc..)

GeckoEidechse commented 1 year ago

Compared to parsing JSON and hoping it's not formatted horribly for whatever reason.

That's what you have libraries for :P That being said, I'm fine with parsing a statically text file by hand ^^

Could we discuss the uri link handling?

What specifically do you have in mind? The ror2mm:// from Thunderstore?

GeckoEidechse commented 1 year ago

I was thinking that if 1 manager wanted to become the main handler, should we check for other installs of other managers?.(if viper wanted to handle links but r2mm existed, r2mm would be default. Etc..)

Personally I'm not planning the to add one cause the current Thunderstore mod integration in FlightCore turned out to be more/as usable as Thunderstore website itself. The way I know URI handlers, if there are multiple registered, you'll be asked to pick one each time. At least that's my experience with URIs in Firefox. So it might just be fine to register anyway and the user will get the option or it will override it, idk.

I'd suggest continuing URI handling discussion in #124 as ebkr who develops r2mm also commented there about it.

0neGal commented 1 year ago

I'm pretty sure all of is here can generate a JSON file just fine though....

Given you've dealt with the Thunderstore and mod installing, I would assume you've also dealt with how terribly people can mess up something as simple as JSON. The problem is not really that JSON parsing is hard, the problem is that going from basic plaintext to a format like JSON adds far more potential for edge cases, and possible errors that can occur.

Could we discuss the uri link handling?. I was thinking that if 1 manager wanted to become the main handler, should we check for other installs of other managers?.(if viper wanted to handle links but r2mm existed, r2mm would be default. Etc..)

We've actually had an issue for that before (#124), and my opinion from it remains the same as at the time, and just to sum it up here:

I don't think URI handlers are bad, however we'd need a new handler at not something like ror2mm:// it makes no sense to attempt to hijack that handler, even if the purpose is the same and your browser may prompt you for which to use (although I don't think Chromium browsers prompt, but Firefox does iirc)

Changing the handler however has backwards compatibility problems...

Furthermore, I don't see the biggest reasoning behind having such a handler, especially with Viper's mod browser effectively being equivalent to browsing the Thunderstore web page, unless it's because you've been sent a link and you want to quickly open it.

But I am not directly against adding it, if done properly.

GeckoEidechse commented 1 year ago

I'm pretty sure all of is here can generate a JSON file just fine though....

Given you've dealt with the Thunderstore and mod installing, I would assume you've also dealt with how terribly people can mess up something as simple as JSON. The problem is not really that JSON parsing is hard, the problem is that going from basic plaintext to a format like JSON adds far more potential for edge cases, and possible errors that can occur.

I found that it tends to just be JSON comments or trailing commas that are the issues. Given that those are all in spec for JSON5 I just switched my JSON parser with one that supports JSON5 and so far didn't have any issues since ^^

(Between URI handling and this, we're starting to stray off-topic from the original here btw :P)

0neGal commented 1 year ago

I found that it tends to just be JSON comments or trailing commas that are the issues. Given that those are all in spec for JSON5 I just switched my JSON parser with one that supports JSON5 and so far didn't have any issues since ^^

While JSON5 is still a great idea, and also was when I first heard about it, it's major problem is backwards compatibility, and in our case as well, interoperability, not staying to the widely used spec/parsers means other launchers/managers will now either update to JSON5 as well, or repair the JSON themselves, once again, more possibilities for edge cases and errors.

So it's far more ideal for everybody, to make it try and parse with a wider supported spec, and repair if able, and in any other case throw an error, as to not cause any other problems.

Between URI handling and this, we're starting to stray off-topic from the original here btw :P

Mostly agree here, on that off-off-topic perhaps the R2NorthstarTools org should have a sample repo just to contain mod manager discussions or just discussions overall.

GeckoEidechse commented 1 year ago

While JSON5 is still a great idea, and also was when I first heard about it, it's major problem is backwards compatibility, and in our case as well, interoperability, not staying to the widely used spec/parsers means other launchers/managers will now either update to JSON5 as well, or repair the JSON themselves, once again, more possibilities for edge cases and errors.

In the end what matters is whether Northstar supports it, cause that's what's needed for Northstar to load the mod and also what most mod developers will use for "testing". rapidjson which Northstar uses, seems to support comments in JSON just fine for example from past experience. And it did so from the beginning.

So I don't see any need to "repair" JSON files which are supported by Northstar anyway. I feel it's rather the responsibility of the mod-manager author to ensure they are still able to parse the JSON file for their own needs. Again using JSON5 libs seem to deal with it just fine and if it they don't, rapidjson from Northstar likely doesn't either and that point the mod author have just shipped a broken mod and will have to fix it themselves. ¯\_(ツ)_/¯

TL;DR: Assume that what Northstar supports is the "standard" and just adhere to that instead of trying to ensure everything is vanilla JSON.

AnActualEmerald commented 1 year ago

I'd be in favor of a plain thunderstore_author.txt I think

Perhaps for extensibility we could keep that file and any future similar files in a specific directory? managed/ or something like that?

GeckoEidechse commented 1 year ago

Perhaps for extensibility we could keep that file and any future similar files in a specific directory? managed/ or something like that?

In that case I would try to make it somewhat obvious by whom (so mod-managers) it's being managed in the naming or by adding a README,

Honestly, I'd just stick to the single thunderstore_author.txt file ^^

0neGal commented 1 year ago

Honestly, I'd just stick to the single thunderstore_author.txt file ^^

Agree with this, even tho I get the idea of making folders, I just overall would like to make the least amount of changes to the mod folder/files but yeah

GeckoEidechse commented 1 year ago

Aight, so are we in favour of a single thunderstore_author.txt added to the corresponding Northstar mod folder?

:+1: / :-1: ?

GeckoEidechse commented 1 year ago

Based on votes image thunderstore_author.txt it is then ^^

0neGal commented 1 year ago

Lovely! I'll however leave the issue open for now, and close it once the author file is implemented into Viper.

GeckoEidechse commented 1 year ago

Just to be clear and that everyone is on the same page. thunderstore_author.txt is an additional file added by mod managers, meaning to uniquely identify the corresponding Thunderstore mod, mod-managers need to add both manifest.json (located inside the zip of the Thunderstore mod folder) and thunderstore_author.txt (contains author name as discussed above, author name has to be manually extracted from mod-string/zip filename/whatever by mod-manager).

Using Fifty-NorthstarLoadingAnim-2.0.0 as an example, the folder structure after a mod-manager installed the mod would look like

.
├─── mod/                     <-- from the Northstar mod
├─── paks/                    <-- from the Northstar mod
├─── manifest.json            <-- added by mod manager
├─── mod.json                 <-- from the Northstar mod
└─── thunderstore_author.txt  <-- added by mod manager
GeckoEidechse commented 1 year ago

For FlightCore this new standard is now implemented in https://github.com/R2NorthstarTools/FlightCore/pull/133 thanks to the changes done in libthermite.

0neGal commented 1 year ago

The bb25cfe commit more or less implements this into Viper, however currently it only makes the file, actually using it will be made soon as well, however it was less of a priority.

With that I'll close this issue, feel free to re-open if anything else warrants discussion or similar!