search

KallistiOS / kos-ports

Ported library collection for KallistiOS
Other
57 stars 35 forks source link

Updating SDL keyboard to use new KOS API. #70

Closed gyrovorbis closed 1 month ago

gyrovorbis commented 6 months ago

This needs to be merged as soon as https://github.com/KallistiOS/KallistiOS/pull/503 goes live to make it work with the SDL kos-port.

QuzarDC commented 2 months ago

@gyrovorbis Wanna use this as an opportunity to flex the new KOS version testing functionality? That way the update won't break people coming in on v2.1.0.

Though, now that makes me realize we'd want to do stuff like try to test ports changes against versions, but that should rarely be much of an issue.

gyrovorbis commented 2 months ago

@gyrovorbis Wanna use this as an opportunity to flex the new KOS version testing functionality? That way the update won't break people coming in on v2.1.0.

Though, now that makes me realize we'd want to do stuff like try to test ports changes against versions, but that should rarely be much of an issue.

Not a bad idea! Done!

ppxxcc commented 1 month ago

I really don't want to be snarky but would it be too much trouble for the authors of PRs to spend the 10 seconds to rebuild and make sure that the source code which has been modified actually builds? Presumably you are already sitting there at your computer with the modified source code ready because how else would you have just pushed a change to your local repo which you are about to submit?

This isn't the first time when blatantly untested changes (which don't even build, should be so obvious!) broke KOS or kos-ports for everybody else.

gyrovorbis commented 1 month ago

The issues with the versioning system (which were reviewed by half a dozen developers and wasn't caught by anyone, so sue us) was totally fixed here: https://github.com/KallistiOS/kos-ports/pull/70, which I was expecting to get merged first.

Almost like CI would ensure this would never happen again or something.

My mistake was not communicating that the other PR needed to be merged first due to this issue, not "zomg bro never even built this thing."

QuzarDC commented 4 weeks ago

The issues with the versioning system (which were reviewed by half a dozen developers and wasn't caught by anyone, so sue us) was totally fixed here: #70, which I was expecting to get merged first.

Almost like CI would ensure this would never happen again or something.

My mistake was not communicating that the other PR needed to be merged first due to this issue, not "zomg bro never even built this thing."

70 is this PR. Which were you meaning needed to be merged first?

darcagn commented 4 weeks ago

The issues with the versioning system (which were reviewed by half a dozen developers and wasn't caught by anyone, so sue us) was totally fixed here: #70, which I was expecting to get merged first. Almost like CI would ensure this would never happen again or something. My mistake was not communicating that the other PR needed to be merged first due to this issue, not "zomg bro never even built this thing."

70 is this PR. Which were you meaning needed to be merged first?

I believe he means https://github.com/KallistiOS/KallistiOS/pull/503 which has changes to version.h. But this being merged would not solve the situation, because version.h would still be broken in KOS stable, breaking SDL compilation. Before the SDL kos-port can be updated I believe the version.h fix would need to be backported to stable and a new v2.1.1 tag released.