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

Ports: Add Sonic Mania Port for SerenityOS. Last Pull request try. #24325

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

LuisYeah1234-hub
Copy link

No description provided.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 14, 2024
@BuggieBot
Copy link
Member

Hello!

One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

Comment on lines +32 to +33
run mv ../stb-5736b15f7ea0ffb08dd38af21067c314d6a3aae9/ dependencies/all/stb_vorbis
run mv ../tinyxml2-10.0.0 dependencies/all/tinyxml2/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's up with these vendored dependencies? Why aren't we referencing ports for stb_vorbis and tinyxml2?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required for it to compile

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these really can't be made work using the on-system libraries, I'd at least recommend going through git+ so that submodules are retrieved automatically.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have a stb port and I can't think of a reason why tinyxml2 should not be portable as well.

Comment on lines +39 to +40
run_nocd mkdir -p ${SERENITY_INSTALL_ROOT}/home/anon/RSDKv5
run cp -RT bin/Serenity/SDL2/Game.so ${SERENITY_INSTALL_ROOT}/home/anon/RSDKv5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does sonicmania go into /usr/local/share/games, but this .so file goes into /home/anon/RSDKv5? Shouldn't it go into /usr/local/ ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default the Game checks for Game.so in RSDKv5 folder already tried in usr/local and game folder it broke

'stb'
)
files=(
"https://github.com/Rubberduckycooly/RSDKv5-Decompilation/archive/1b01682d22948aaa3971e96f20f038c144b7eaf4.tar.gz#b12ec54e5d763a4a48b1f4a3ca19cca4441c6efdaa95d77614dc82d1ab11b916"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we downloading an arbitrary commit of this package instead of a tagged version?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right we are downloading a specific commit i'll look into it later


install() {
run_nocd mkdir -p ${SERENITY_INSTALL_ROOT}/usr/local/share/games/sonicmania/
run cp -RT bin/Serenity/SDL2/RSDKv5U ${SERENITY_INSTALL_ROOT}/usr/local/share/games/sonicmania/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't there am install rule for this file in the upstream build?

i.e. why is make install not enough?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

install function for make install does not seem to exist in this repo makefile i'll check more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants