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.

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-music tarballs created are incompatible with OpenTTD client #59

Closed
nielsmh opened this issue Jul 28, 2020 · 8 comments
Closed

Base-music tarballs created are incompatible with OpenTTD client #59

nielsmh opened this issue Jul 28, 2020 · 8 comments

Comments

@nielsmh
Copy link

@nielsmh nielsmh commented Jul 28, 2020

Based on https://www.tt-forums.net/viewtopic.php?p=1234714#p1234714

A ZIP file with a music set uploaded to BaNaNaS had the directory in the root of the ZIP file stripped, so all the contents are at the root of the generated TAR file. This causes OpenTTD to discard the downloaded file (see OpenTTD/OpenTTD#8283) as invalid, so while the download at first appears to have worked, it actually failed.

@TrueBrain TrueBrain transferred this issue from OpenTTD/bananas-server Jul 29, 2020
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jul 29, 2020

As a FYI: bananas-server is only serving the content for the in-game client. The bananas-api handles uploads etc. So I moved this ticket.

I will need to investigate this, as this should never be possible: https://github.com/OpenTTD/bananas-api/blob/master/bananas_api/new_upload/session_publish.py#L69

tar_path is never empty, and it forces all files to be in at least a single path. So this will be interesting to trace back :)

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jul 30, 2020

The initial diagnostic is incorrect. The content of the tar is like:

Rise_of_the_Triad_OST_NST-1.0/00_chant_theme.mid
Rise_of_the_Triad_OST_NST-1.0/02_riseofthetriad.mid
Rise_of_the_Triad_OST_NST-1.0/04_godrestyedeadlygentlemen.mid

There is a root-folder there. And the tarball looks the same as other (hopefully working) base musics. Not sure what is going on, will investigate a bit further.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jul 30, 2020

Okay, this problem is a bit more subtle:

OpenTTD assumes that the very first entry in a base music tarball (and only for base music, as far as I can tell) is a "directory" entry, stating the initial directory. What makes this a bit tricky, that it heavily depends on the tar-specs if this is even allowed.

The original tar-format could only add files, symlinks or hardlinks. I believe ustar added support for these node types. For the new BaNaNaS, we could not find any (documented) reason to use ustar or pax, so we went with the oldest of all (well, ustar, but without using the additional stuff), not sure what OpenTTD could handle. This means there is no entry to indicate the directory as first entry in a tarball.

For all content-downloads, using the old-format saves a few bytes and is easier to parse. But most downloads are not extracted by the client. Only base music is, I believe. So this is why this only happens for base music.

So, this is a two-edged sword. There is nothing really wrong with the tarball, it is just that OpenTTD has a weird assumption on how it is created. This not-documented discrepancy between the OpenTTD client and BaNaNAS is what is breaking this.
Fixing OpenTTD by looking for the directory based on the filename would be the clean solution, but that would leave all current clients incompatible. So fixing BaNaNaS is the most compatible solution, but re-introduces an odd way of using tarballs. Assuming the first entry is a directory-entry with the prefix name, is a bit of a weird requirement to put onto tarballs.

But enfin, I will try to fix this on the BaNaNaS side, and repackage this tarball.

I would love for someone to validate my findings, especially that only base-music is affected by this.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jul 30, 2020

Minor addition:

https://github.com/OpenTTD/bananas-api/blob/master/bananas_api/new_upload/session_publish.py#L70

Turns out we already pack with ustar, which supports directories. So I guess it is only a matter of explicitly adding the directory as an entry.

More additions: uploads via musa also do not add a directory entry, so they also broke for base-music.

And just to confirm: this has nothing to do with how it was uploaded etc. This resulting tarball will always miss the initial directory entry the OpenTTD client is expecting.

@TrueBrain TrueBrain changed the title Directories in uploaded ZIP files stripped Base-music tarballs created by the API are incompatible with OpenTTD client Jul 30, 2020
@TrueBrain TrueBrain changed the title Base-music tarballs created by the API are incompatible with OpenTTD client Base-music tarballs created are incompatible with OpenTTD client Jul 30, 2020
TrueBrain added a commit to TrueBrain/OpenTTD-bananas-api that referenced this issue Sep 2, 2020
…arballs

This is only the case during extraction, which only happens for
Base Music. It is a silly thing of the OpenTTD's client, but we
cannot change history, so we better produce tarballs that work for
older clients too.
@TrueBrain TrueBrain closed this in 4e96218 Sep 2, 2020
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Sep 2, 2020

This problem should now be resolved, and any new uploads should work now.

The current uploaded file does not work; I am reluctant to manually fix the issue, so I see two possibilities:

  1. we remove the current upload completely
  2. the author uploads a new version, which does require some changes in some file before the upload will be accepted.

The second option would be easiest, with the least amount of risk, but I have no problem diving into the first. Just let me know if that is needed :)

@FilmBoy84
Copy link

@FilmBoy84 FilmBoy84 commented Sep 9, 2020

@TrueBrain Im more than happy to update the file
What exactly needs changing?

TrueBrain added a commit to TrueBrain/OpenTTD-BaNaNaS that referenced this issue Sep 9, 2020
…tly changed

See OpenTTD/bananas-api#59 for more
details on what was broken and what is fixed. Normally we do not
change these things for users, but this was a bug in BaNaNaS,
with a trivial way to fix it for the user. Hence this route.
TrueBrain added a commit to TrueBrain/OpenTTD-BaNaNaS that referenced this issue Sep 9, 2020
…tly changed

See OpenTTD/bananas-api#59 for more
details on what was broken and what is fixed. Normally we do not
change these things for users, but this was a bug in BaNaNaS,
with a trivial way to fix it for the user. Hence this route.
TrueBrain added a commit to OpenTTD/BaNaNaS that referenced this issue Sep 9, 2020
…tly changed (#65)

See OpenTTD/bananas-api#59 for more
details on what was broken and what is fixed. Normally we do not
change these things for users, but this was a bug in BaNaNaS,
with a trivial way to fix it for the user. Hence this route.
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Sep 9, 2020

@FilmBoy84 : I realised it might be hard for you to fix, and I did not realise before how easy it would be for me to fix. So I just fixed the upload in the backend. You can now download Rise of the Triad OST NST in-game without issue :) No further action required from your part!

w00p :)

@FilmBoy84
Copy link

@FilmBoy84 FilmBoy84 commented Sep 11, 2020

That's fantastic, many thanks for sorting, can confirm it works
Will update my forum thread accordingly to say BaNaNaS now allows download

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.