fix contrib build on linux and macos (nsf2wav, nsfmeta), add contrib to github workflow

[tsone]

Minor changes to fix contrib build under macOS. Didn't test but might work for Linux and other GNU/posix too (if not, probably only minor changes are needed to fix that).

@bbbradsmith I needs a NSF file with old Shift JIS text to complete this. Do you have some?

[bbbradsmith]

If you haven't tested this for Linux already I can't possibly accept it. Please do this first. Even better would be a github actions workflow that could demonstrate this builds correctly on some intended platforms (I would suggest: Ubuntu, MacOS, Windows MinGW). That way this doesn't need to even be a question in the future.

To be honest, I regret allowing the "contribs" folder at all. I don't know what code is in it or what it does, so it has been a problem to maintain. NSFPlay 2 is a Windows-only project.

NSFPlay 3, will support Windows, Linux and MacOS at the same time, including a command line version, so this should be obsolete at that point. I've basically already done the cross-platform framework for it, but I don't know when I'll have time to actually proceed with the actual main body of code for it.

Regarding nsf.cpp please use the same tab convention as the surrounding code. (The original code was not always consistent, unfortunately, but at least within the same function we can be consistent.)

[bbbradsmith]

As for an NSF containing Shift-JIS it seems a lot of websites have decayed so I couldn't find the files I would normally link to. Here's one I found in Mr. Norbert's NSF collection:
ダイナマイトボウル.zip
The result should show:
Title: ダイナマイトボウル
Artist: ﾀﾁﾁﾃﾄﾅﾆﾇﾈﾉﾊﾋﾌﾍﾎﾏ

[tsone]

If you haven't tested this for Linux already I can't possibly accept it. Please do this first. Even better would be a github actions workflow that could demonstrate this builds correctly on some intended platforms (I would suggest: Ubuntu, MacOS, Windows MinGW). That way this doesn't need to even be a question in the future.

To be honest, I regret allowing the "contribs" folder at all. I don't know what code is in it or what it does, so it has been a problem to maintain. NSFPlay 2 is a Windows-only project.

I.e. an automatic CI build (github action etc.) for the contrib build would be great. Let me see what I can do.

NSFPlay 3, will support Windows, Linux and MacOS at the same time, including a command line version, so this should be obsolete at that point. I've basically already done the cross-platform framework for it, but I don't know when I'll have time to actually proceed with the actual main body of code for it.

Do you have a timeline for this? Not trying to press you or anything, I'm just very interested.

Regarding nsf.cpp please use the same tab convention as the surrounding code. (The original code was not always consistent, unfortunately, but at least within the same function we can be consistent.)

Sorry, I thought my IDE handled that automatically. I'll fix.

[bbbradsmith]

I.e. an automatic CI build (github action etc.) for the contrib build would be great. Let me see what I can do.

NSFPlay 3's build can be used as an example, though this one would be a lot less complicated. The branches should be "master", and there is no need for the submodule/caching steps so that should simplify the jobs significantly. Please put it in a separate yml, maybe .github/workflows/contribs.yml or something. I don't think workflows will build/test in a PR itself, but they should run on your own fork if you are able to put it in your master branch.

https://github.com/bbbradsmith/nsfplay/blob/nsfplay3/.github/workflows/build_nsfplay3.yml

Do you have a timeline for this? Not trying to press you or anything, I'm just very interested.

Sorry, I don't really have a timeline for anything in my life right now, and volunteer open source projects are a somewhat low priority.

[bbbradsmith]

I would recommend using MSYS2 + MinGW rather than attempting to just run make on the Windows runners.

Setup is something like:

 build-msys2:
    name: Windows MSYS2
    runs-on: windows-latest
    steps:
      - name: Checkout Files
        uses: actions/checkout@v4
      - name: Setup MSYS2
        uses: msys2/setup-msys2@v2
        with:
          msystem: UCRT64
          install: make mingw-w64-ucrt-x86_64-gcc mingw-w64-i686-gcc

Also, please put these in a separate contribs.yml rather than appending it to build.yml

[tsone]

Thanks. Also the Linux build was broken. The -liconv linker flag broke it. The flag is unnecessary as iconv is now part of glibc. I fixed this and the Linux build works now.

If possible it would be good to have even failed contrib builds status for the main branch. If the current commit is broken, those interested can easily find the last main branch commit that worked (from the green dot in the commits list). This avoids people try to clone and fail building and perhaps even bisect (which I did). So it would be great help to have them on for main branch. I think that would be then build.yml. What do you think?

[tsone]

I would recommend using MSYS2 + MinGW rather than attempting to just run make on the Windows runners.

Because of the iconv dependency I don't think contrib can be built on Windows currently. I don't think it's a big deal as nsfplay itself can be used on Windows... But do you want the contrib programs to build on Windows? That can be done.

Do you allow having the .nsf file you sent previously as a functional test case as part of the contrib workflow? I could add a simple test the Shift JIS is read correctly. I could add the .nsf under contrib/test/ and access it there.

[bbbradsmith]

I would recommend using MSYS2 + MinGW rather than attempting to just run make on the Windows runners.
Because of the iconv dependency I don't think contrib can be built on Windows currently. I don't think it's a big deal as nsfplay itself can be used on Windows... But do you want the contrib programs to build on Windows? That can be done.

As a general request, please avoid force pushing to PRs. The commit history can normally be dealt with via square merging, or I could ask for you to revise/rebase it with a final push once the details are worked out, but for discussion of a PR it makes it much difficult to follow the history of conversation.

Also, if you're doing CI testing, the best way to do this is on your own branch first until you have it working, then copy the changes here to your PR. That way I am not getting notifications over and over about failed builds.

Anyway, despite the force push, I managed to dig up the [url=https://github.com/bbbradsmith/nsfplay/actions/runs/12982403539/workflow]last Windows workflow attempted[/url]. What I was suggesting is to use MSYS2 + MinGW. MSYS2 should have the iconv library it needs.

If you check workflow I linked above as an example (link) the build-msys2 job should show how to do with. Essentially you just need the msys2/setup-msys2@v2 step where you list the required packages (iconv should be one of them) after which the build should use the msys2 shell where you can run make like the other two platforms.

Finally, as requested above, please put the contribs worflow in a separate yml file, not in the same one as the NSFPlay build.

Do you allow having the .nsf file you sent previously as a functional test case as part of the contrib workflow? I could add a simple test the Shift JIS is read correctly. I could add the .nsf under contrib/test/ and access it there.

No, I don't wish to do this. I don't want to add any more maintenance burden to the contribs code than there already is. For the time being, please test it locally.

NSFPlay 3 will have a place for this kind of thing. For NSFPlay 2 it is not worth it.

[bbbradsmith]

Though, looking at the change, despite MSYS2 + MinGW having an iconv library, it is probably best for it just to use the existing windows library version, since it has that too. No need to enlist iconv for that build.

[tsone]

No, I don't wish to do this. I don't want to add any more maintenance burden to the contribs code than there already is.

I understand but I don't believe maintenance burden would be so high since nsfplay 3 would soon replace these programs. The gain is more than potential downside. It would be beneficial to have them before nsfplay 3 is released.

Though, looking at the change, despite MSYS2 + MinGW having an iconv library, it is probably best for it just to use the existing windows library version, since it has that too. No need to enlist iconv for that build.

nsf2wav and nsfmeta are originally for non-Windows users who can't use nsfplay. They are for posix, and porting them to Windows changes their original intent, which requires more testing. This is outside the scope of this PR.

However, one could do this in a follow up through implementing a portable mbs_to_utf8(), which would for example remove class ToUTF8 in nsfmeta.cpp and clean up sjis_legacy(). However, I won't be doing this because I don't think you consider contrib so important as it's not part of the GitHub CI workflow.

But going back to this PR, I'll test the programs with Shift JIS encoding, perhaps synthetic test .nsf files generated through a python script. Do you have such script?

[bbbradsmith]

I understand but I don't believe maintenance burden

Test code is more code to maintain, and for what you're proposing I don't believe there is enough practical benefit to justify it. Please don't add this to the PR.

nsf2wav and nsfmeta are originally for non-Windows users who can't use nsfplay. They are for posix, and porting them to Windows changes their original intent, which requires more testing. This is outside the scope of this PR.

They have separate functionality from NSFplay, and there is already demonstrated prior use on Windows.

Your PR is essentially requesting MacOS support. What I'm asking for is to add CI support for the 2 established platforms, and now MacOS, so that we can more quickly resolve changes to the makefile that submitters can't test on the other platforms. That is why I think this PR should include such a thing, it's not really a separate matter from adding MacOS support.

Are you having trouble creating an MSYS2 build action? I can assist if you really can't get it working. Sorry to keep asking, but I didn't find in the build history any attempts to do so, only an attempt to do it with plain Windows, which should be expected to fail without MSYS2. As I said before, MSYS2 does have an available iconv library so this should not be a sticking point.

However, one could do this in a follow up through implementing a portable mbs_to_utf8(), which would for example remove class ToUTF8 in nsfmeta.cpp and clean up sjis_legacy(). However, I won't be doing this because I don't think you consider contrib so important as it's not part of the GitHub CI workflow.

I don't understand the last comment. I've already specifically requested a CI workflow for contrib. What do you mean?

However, yes in general I don't think we should pursue perfecting this code unless it it results in practical gainm but I think supporting MacOS is useful, and I think preventing future makefile tennis with a cross-platform CI is useful.

NSFPlay 3 is already written to convert shift-jis in a cross-platform way, and abstracts away the non-utf Windows issue, so any solutions relevant to NSFPlay 2 or its contribs probably won't be relevant or transferrable.

Returning to the relevant question, though, I'm curious how does nsfmeta currently avoids sjis_legacy() causing a Windows dependency problem currently? It seems to use LoadFile from xgm/player/nsf/nsf.cpp? (Edit: I see that your changes define around it.)

But going back to this PR, I'll test the programs with Shift JIS encoding, perhaps synthetic test .nsf files generated through a python script. Do you have such script?

No, sorry, I don't have a script for generating synthetic test NSF files.

[bbbradsmith]

Okay, I have added the MSYS2 build step. With that completed I think this can be closed.

I think the problem was that the contribs were originally intended for MSYS2, not Linux or MacOS. Nobody made that clear in the documentation, but with the build test in place I think we should be able to use it on all three.

If you do further testing and find a problem with the unicode, or something else please do open an issue or PR.

[tsone]

Thanks for the merge! 🎉