jpage8580 / GTUltra

GTUltra - Extensively modified GoatTracker Stereo (2.76) version. With many new features. See the GTUltra.PDF file for full details
GNU General Public License v2.0
88 stars 10 forks source link

1.5.1 does not build on non-windows #57

Closed Zirias closed 1 year ago

Zirias commented 1 year ago

I'm just trying to upgrade the FreeBSD port of GTUltra, unfortunately I can't get it to build. The errors seem all related to gt2reloc.c.

The first issue is usage of keybd_event(), which is a Windows-specific function. I don't really understand the purpose of "faking" keystrokes here, but just removing it for non-Windows helps:

@@ -219,10 +219,12 @@ int main(int argc, char **argv)
        }
        else {
                usage();
+#ifdef __WIN32__
                // ENTER key down
                keybd_event(VK_RETURN, 0x9C, 0, 0);
                // ENTER key up
                keybd_event(VK_RETURN, 0x9C, KEYEVENTF_KEYUP, 0);
+#endif
                exit(-1);

        }
@@ -452,10 +454,12 @@ int main(int argc, char **argv)
        // perform relocation
        relocator(&gtObject, 1);

+#ifdef __WIN32__
        // ENTER key down
        keybd_event(VK_RETURN, 0x9C, 0, 0);
        // ENTER key up
        keybd_event(VK_RETURN, 0x9C, KEYEVENTF_KEYUP, 0);
+#endif
        // Exit
        exit(0);
        return 0;

The next issue is a linker complaint about double definition of gtObject. I have to really start guessing here, my guess is it should reference an object defined somwhere else, so the following patch would help if this assumption would be correct:

@@ -95,7 +95,7 @@ int useOriginalGTFunctionKeys = 0;
 char debugTextbuffer[MAX_PATHNAME];

 int useOriginalGTFunctionKeys = 0;
-GTOBJECT gtObject;
+extern GTOBJECT gtObject;
 int useRepeatsWhenCompressing = 1;
 char infoTextBuffer[256];
 int midiEnabled = 0;

But then, I really have no idea about the next error, linking gt2reloc complains about an undefined symbol playUntilEnd, referenced by gorder.o. This one is defined in gt2stereo.o, but linking that as well creates a whole list of duplicate symbols instead.

I'll hold back the update of the FreeBSD port for now and hope for some feedback, because I don't want to build/package a broken version of GTUltra ... TIA!

jpage8580 commented 1 year ago

Thanks for this. And - sorry!

Have pushed changes. Can you try again now?

On Mon, May 22, 2023 at 6:17 PM Felix Palmen @.***> wrote:

I'm just trying to upgrade the FreeBSD port of GTUltra, unfortunately I can't get it to build. The errors seem all related to gt2reloc.c.

The first issue is usage of keybd_event(), which is a Windows-specific function. I don't really understand the purpose of "faking" keystrokes here, but just removing it for non-Windows helps:

@@ -219,10 +219,12 @@ int main(int argc, char **argv) } else { usage(); +#ifdef WIN32 // ENTER key down keybd_event(VK_RETURN, 0x9C, 0, 0); // ENTER key up keybd_event(VK_RETURN, 0x9C, KEYEVENTF_KEYUP, 0); +#endif exit(-1);

    }

@@ -452,10 +454,12 @@ int main(int argc, char **argv) // perform relocation relocator(&gtObject, 1);

+#ifdef WIN32 // ENTER key down keybd_event(VK_RETURN, 0x9C, 0, 0); // ENTER key up keybd_event(VK_RETURN, 0x9C, KEYEVENTF_KEYUP, 0); +#endif // Exit exit(0); return 0;

The next issue is a linker complaint about double definition of gtObject. I have to really start guessing here, my guess is it should reference an object defined somwhere else, so the following patch would help if this assumption would be correct:

@@ -95,7 +95,7 @@ int useOriginalGTFunctionKeys = 0; char debugTextbuffer[MAX_PATHNAME];

int useOriginalGTFunctionKeys = 0; -GTOBJECT gtObject; +extern GTOBJECT gtObject; int useRepeatsWhenCompressing = 1; char infoTextBuffer[256]; int midiEnabled = 0;

But then, I really have no idea about the next error, linking gt2reloc complains about an undefined symbol playUntilEnd, referenced by gorder.o. This one is defined in gt2stereo.o, but linking that as well creates a whole list of duplicate symbols instead.

I'll hold back the update of the FreeBSD port for now and hope for some feedback, because I don't want to build/package a broken version of GTUltra ... TIA!

— Reply to this email directly, view it on GitHub https://github.com/jpage8580/GTUltra/issues/57, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYGMDBT2B7WUMB2H73CVTGLXHONRXANCNFSM6AAAAAAYKXRJKI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Zirias commented 1 year ago

Thanks a lot (and that was really quick!), pulling in your latest commit as a patch makes the port build again!

So, I'll continue working on updating the port, 1.5.1 will probably be committed to FreeBSD's ports collection later today.

jpage8580 commented 1 year ago

Great! The keyboard code - All I wanted was to do a printf!! But SDL2 doesn't allow that so I had to use their debug log commands to get anything to display. However, that then sets things up in a way that requires return to be pressed to quit.. Use case was someone who wanted this called in a batch, repeatedly - hence my hacks!

On Mon, May 22, 2023 at 7:39 PM Felix Palmen @.***> wrote:

Thanks a lot (and that was really quick!), pulling in your latest commit as a patch makes the port build again!

So, I'll continue working on updating the port, 1.5.1 will probably be committed to FreeBSD's ports collection later today.

— Reply to this email directly, view it on GitHub https://github.com/jpage8580/GTUltra/issues/57#issuecomment-1557714229, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYGMDBW5NCAISC6ULS7VRV3XHOXHPANCNFSM6AAAAAAYKXRJKI . You are receiving this because you commented.Message ID: @.***>

Zirias commented 1 year ago

That sounds completely b0rked. Better analyze what SDL is doing here with the standard file descriptors?

Anyways, doesn't look like the result without that hack will be completely broken, so I'll update that port now (installing gt2reloc as gt2relocu because the goattracker port already installs a binary by the same name).

It certainly looks good now: https://home.palmen-it.de/testbuilder/data/14a-default/2023-05-22_20h54m19s/logs/gtultra-1.5.1.log (might have a look at all these warnings one day, hehe)

Currently running another test build on FreeBSD 12 (oldest still supported release) and i386 ... if that's fine as well, go for it. Thanks again!

jpage8580 commented 1 year ago

I don't see any of those warnings. My build options hide them from me - But jeeeezzz, they're shocking! haha! The issue is due to changes in SDL2 - It worked fine in SDL!

On Mon, May 22, 2023 at 8:01 PM Felix Palmen @.***> wrote:

That sounds completely b0rked. Better analyze what SDL is doing here with the standard file descriptors?

Anyways, doesn't look like the result without that hack will be completely broken, so I'll update that port now (installing gt2reloc as gt2relocu because the goattracker port already installs a binary by the same name).

It certainly looks good now: https://home.palmen-it.de/testbuilder/data/14a-default/2023-05-22_20h54m19s/logs/gtultra-1.5.1.log (might have a look at all these warnings one day, hehe)

Currently running another test build on FreeBSD 12 (oldest still supported release) and i386 ... if that's fine as well, go for it. Thanks again!

— Reply to this email directly, view it on GitHub https://github.com/jpage8580/GTUltra/issues/57#issuecomment-1557751751, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYGMDBWYFTYE4ND2OQR66KLXHOZYHANCNFSM6AAAAAAYKXRJKI . You are receiving this because you commented.Message ID: @.***>

Zirias commented 1 year ago

I like to enable all possible warnings when coding in C or C++ (like, -Wall -Wextra -pedantic) and even test with both clang and gcc. Although there are many "false positives", getting your code free of warnings really helps avoid actual bugs ...

But anyways, all build tests are fine now, so FreeBSD has the latest GTUltra version now :+1: https://www.freshports.org/audio/gtultra/ https://cgit.freebsd.org/ports/commit/?id=083957ec3fa82fef7b934b50e6dbec384119b253

If only every upstream would fix issues THAT quickly, haha!

jpage8580 commented 1 year ago

Thanks so much for doing this

On Mon, May 22, 2023 at 8:27 PM Felix Palmen @.***> wrote:

I like to enable all possible warnings when coding in C or C++ (like, -Wall -Wextra -pedantic) and even test with both clang and gcc. Although there are many "false positives", getting your code free of warnings really helps avoid actual bugs ...

But anyways, all build tests are fine now, so FreeBSD has the latest GTUltra version now 👍 https://www.freshports.org/audio/gtultra/

https://cgit.freebsd.org/ports/commit/?id=083957ec3fa82fef7b934b50e6dbec384119b253

If only every upstream would fix issues THAT quickly, haha!

— Reply to this email directly, view it on GitHub https://github.com/jpage8580/GTUltra/issues/57#issuecomment-1557818226, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYGMDBSVZP5Y2KWFYP4RXG3XHO4YPANCNFSM6AAAAAAYKXRJKI . You are receiving this because you commented.Message ID: @.***>

Zirias commented 1 year ago

TBH, I have to thank you.

I see this code still has some quality issues (this wall of warnings is unsettling, I won't promise anything, but if I find some time, I might have a look), but I guess most of it is inherited from original goattracker ...

Nevertheless: My personal background is, I'm one of those "C64 kids", I'm sometimes a bit active in the scene as a coder, I even composed music (using trackers as well as my own player code ...) – and at the same time, I'm a "professional" software dev and architect and just love FreeBSD (and made it to a ports committer there quickly).

So, my very personal goal is to make FreeBSD a top notch platform for C64 enthusiasts. And of course, that's only possible with free and open-source software available (that e.g. also allows binary redistribution, users don't want to compile themselves!). Unfortunately, some very useful tools aren't free (SpritePad, CharPad), some others are "free" but not open-source (DirMaster), so you can't build and package them for FreeBSD. Concerning D64 editing, I tried to fill the gap myself with "v1541commander", and I should really get back working on it! But hey, always too many private projects ;)

Long story short, I'm very thankful for any free and open-source software around the C64 I can find!

jpage8580 commented 1 year ago

Hopefully, I've just fixed up all of the warnings. Hopefully, without breaking anything in the meantime!

On Mon, May 22, 2023 at 9:38 PM Felix Palmen @.***> wrote:

TBH, I have to thank you.

I see this code still has some quality issues (this wall of warnings is unsettling, I won't promise anything, but if I find some time, I might have a look), but I guess most of it is inherited from original goattracker ...

Nevertheless: My personal background is, I'm one of those "C64 kids", I'm sometimes a bit active in the scene as a coder, I even composed music (using trackers as well as my own player code ...) – and at the same time, I'm a "professional" software dev and architect and just love FreeBSD (and made it to a ports committer there quickly).

So, my very personal goal is to make FreeBSD a top notch platform for C64 enthusiasts. And of course, that's only possible with free and open-source software available (that e.g. also allows binary redistribution, users don't want to compile themselves!). Unfortunately, some very useful tools aren't free (SpritePad, CharPad), some others are "free" but not open-source (DirMaster), so you can't build and package them for FreeBSD. Concerning D64 editing, I tried to fill the gap myself with "v1541commander", and I should really get back working on it! But hey, always too many private projects ;)

Long story short, I'm very thankful for any free and open-source software around the C64 I can find!

— Reply to this email directly, view it on GitHub https://github.com/jpage8580/GTUltra/issues/57#issuecomment-1557953011, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYGMDBVWQLACFA2JBQT36M3XHPFDJANCNFSM6AAAAAAYKXRJKI . You are receiving this because you commented.Message ID: @.***>

Zirias commented 1 year ago

Regression in the commit to fix warnings, see https://github.com/jpage8580/GTUltra/commit/19132bcba908fed2820f54d90e3ec2bfc679f506#diff-56a9aba3cf01edbcf0b5e96f74054c6c9e60b65c72db1cc04064573e854e40ddL458

Result: It again doesn't build on non-windows systems :(

I would really like to understand what exact problem triggered that "workaround" ... IMHO it should be analyzed and properly solved, MAYBE I can help if I find time this weekend ....

jpage8580 commented 1 year ago

I've no idea how that regressed! Have fixed...again...hopefully

This basically explains why I bodged the code...

The original GTreloc (using SDL) has this code:

ifdef WIN32

/ SDL_Init() reroutes stdout and stderr, either to stdout.txt and stderr.txt or to nirwana. simply reopening these handles does, other than suggested on some web pages, not work reliably - opening new files on CON using different handles however does. / STDOUT = fopen("CON", "w"); STDERR = fopen("CON", "w");

endif

This no longer works with SDL2, so I couldn't find any way to get output to the cmd window.

see here: https://stackoverflow.com/questions/14593783/sdl-console-output-works-when-debuging-but-not-when-run-with-the-exe

The only way I could get anything to output was to use SDL_Log. However, using this will leave the cmd window in a state where it wants the used to press return to exit that mode.. This wasn't good for a user case where someone was using gt2reloc in a batch file. So, I basically just added a hack to force the return code!

On Fri, May 26, 2023 at 6:40 PM Felix Palmen @.***> wrote:

Reopened #57 https://github.com/jpage8580/GTUltra/issues/57.

— Reply to this email directly, view it on GitHub https://github.com/jpage8580/GTUltra/issues/57#event-9353198985, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYGMDBROEALJ56VLDBFKO4LXIDTINANCNFSM6AAAAAAYKXRJKI . You are receiving this because you commented.Message ID: @.***>

Zirias commented 1 year ago

Ahh thanks for both the quick fix and some context about the underlying problem! :+1:

As for updating the FreeBSD port, I guess I will just delay it a bit for now (but of course, still check the current tip of the main branch!)

Regarding the issue with SDL.... I hope I will find some time to have a look myself. There just must be some sane way to solve it :open_mouth:

jpage8580 commented 1 year ago

Indeed! My hack was literally to get a version that could be used asap. I could only find ways to solve this that involved recompiling SDL2, which I didn’t want to do.

Silly thing is, the problem only occurs due to gt2reloc using the same code base as GT - even though the actual code to do the export doesn’t really need it. I could just copy the relevant structures and variables into gt2reloc and build it as standalone. But that might then involve a lot more upkeep, keeping both in sync.

On Fri, 26 May 2023 at 19:17, Felix Palmen @.***> wrote:

Ahh thanks for both the quick fix and some context about the underlying problem! 👍

As for updating the FreeBSD port, I guess I will just delay it a bit for now (but of course, still check the current tip of the main branch!)

Regarding the issue with SDL.... I hope I will find some time to have a look myself. There just must be some sane way to solve it 😮

— Reply to this email directly, view it on GitHub https://github.com/jpage8580/GTUltra/issues/57#issuecomment-1564757120, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYGMDBVCIUXJNRHQVLAILBTXIDXVLANCNFSM6AAAAAAYKXRJKI . You are receiving this because you commented.Message ID: @.***>

Zirias commented 1 year ago

Silly thing is, the problem only occurs due to gt2reloc using the same code base as GT - even though the actual code to do the export doesn’t really need it. I could just copy the relevant structures and variables into gt2reloc and build it as standalone. But that might then involve a lot more upkeep, keeping both in sync.

Yeah about this ... and maybe that's indeed the way to happiness ... I have already noticed that the whole codebase could need some sane "structuring".

You certainly would not want to duplicate code, but you would want to separate code needing SDL from code that doesn't (and just use the linker ....), at the very least.

But that's probably a huge amount of work, dissecting the existing code and refactor it into modules (compilation units) that just have one job, as little dependencies as possible, and make sense...

Zirias commented 1 year ago

After the latest commit, it builds fine again, so closing this again, and thanks for that!

jpage8580 commented 1 year ago

Yeah, I’ve already spent substantial time on defining structures to allow for multiple instances of the player code, etc. Wasnt fun, but totally necessary. Maybe worth moving all of those structs into a single header so that both GT editor and reloc could just use that.

On Fri, 26 May 2023 at 19:38, Felix Palmen @.***> wrote:

Silly thing is, the problem only occurs due to gt2reloc using the same code base as GT - even though the actual code to do the export doesn’t really need it. I could just copy the relevant structures and variables into gt2reloc and build it as standalone. But that might then involve a lot more upkeep, keeping both in sync.

Yeah about this ... and maybe that's indeed the way to happiness ... I have already noticed that the whole codebase could need some sane "structuring".

You certainly would not want to duplicate code, but you would want to separate code needing SDL from code that doesn't (and just use the linker ....), at the very least.

But that's probably a huge amount of work, dissecting the existing code and refactor it into modules (compilation units) that just have one job, as little dependencies as possible, and make sense...

— Reply to this email directly, view it on GitHub https://github.com/jpage8580/GTUltra/issues/57#issuecomment-1564782000, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYGMDBX65BUFF2VJPEAWMCLXID2DVANCNFSM6AAAAAAYKXRJKI . You are receiving this because you commented.Message ID: @.***>

jpage8580 commented 1 year ago

Had a good look at removing sdl from gt2reloc last night. It’s everywhere though, as it’s required by the bme library. Not worth the effort just for that stand-alone relocator that isn’t really that necessary anyway

On Fri, 26 May 2023 at 20:13, Jason Page @.***> wrote:

Yeah, I’ve already spent substantial time on defining structures to allow for multiple instances of the player code, etc. Wasnt fun, but totally necessary. Maybe worth moving all of those structs into a single header so that both GT editor and reloc could just use that.

On Fri, 26 May 2023 at 19:38, Felix Palmen @.***> wrote:

Silly thing is, the problem only occurs due to gt2reloc using the same code base as GT - even though the actual code to do the export doesn’t really need it. I could just copy the relevant structures and variables into gt2reloc and build it as standalone. But that might then involve a lot more upkeep, keeping both in sync.

Yeah about this ... and maybe that's indeed the way to happiness ... I have already noticed that the whole codebase could need some sane "structuring".

You certainly would not want to duplicate code, but you would want to separate code needing SDL from code that doesn't (and just use the linker ....), at the very least.

But that's probably a huge amount of work, dissecting the existing code and refactor it into modules (compilation units) that just have one job, as little dependencies as possible, and make sense...

— Reply to this email directly, view it on GitHub https://github.com/jpage8580/GTUltra/issues/57#issuecomment-1564782000, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYGMDBX65BUFF2VJPEAWMCLXID2DVANCNFSM6AAAAAAYKXRJKI . You are receiving this because you commented.Message ID: @.***>