angstsmurf / spatterlight

Updated fork of Spatterlight
GNU General Public License v3.0
105 stars 5 forks source link

Custom patches file for Bocfel #87

Open cspiegel opened 1 month ago

cspiegel commented 1 month ago

For lack of a better place to discuss this...

I'm almost ready to release a new Bocfel, with the only thing holding it up is implementation of a custom patches file as we've briefly discussed elsewhere. As perhaps the only other Bocfel user who will be using this feature, I'd like your feedback on its implementation.

Right now I have it implemented so that you can optionally, during build time, provide a path to a patches file, which will be hard-coded into the binary. Simple and straight forward, but will that cause problems with Mac DMG files? What would be the ideal way to configure this, for you?

angstsmurf commented 1 month ago

It is hard to say what the ideal way would be before I have tried, but what you describe doesn't sound like anything that would pose any particular technical problems.

In theory some people might like to be able to add and remove patches without recompiling, but that is probably more work to implement than it is worth.

EDIT: Looking forward to the new Bocfel version!

cspiegel commented 1 month ago

Let me clarify since my initial comments were a bit ambiguous, and you may have taken a different meaning than I intended:

The path you provide during the build is what's embedded in the binary. Bocfel will try to open that file at runtime. It doesn't embed the patches themselves, just the path to the patch file, so you can add patches without rebuilding.

If that's how you understood it, then never mind! Otherwise, my concern was that in a DMG file, I'm not really sure what happens with a hard-coded path. If it's absolute, is that in a chrooted environment (or similar), where an absolute path is actually relative to the mounted DMG? If not, are you guaranteed a specific working directory, so you could actually embed a relative path (Bocfel doesn't care if it's absolute or relative: it just tries opening whatever it's provided).

I guess the more I think about it, I'm not even certain this would work well on Windows. On the Windows side of things, I provide portable Gargoyle ZIP files, and they look at the location of the gargoyle.exe binary and search relative to that. So my proposal is, perhaps, a non-starter.

What about an environment variable? That feels a little haphazard, but it would be easy for callers to build a path at runtime before starting the binary. Another possibility is a command-line option. That'd be more of a pain to manage in Gargoyle, but that's not a great reason to immediately discount it, either.

angstsmurf commented 1 month ago

All right, I misunderstood. I still don't think this would be a problem. There is already a mechanism in Spatterlight which asks the user for read permission to folders in order to look for images and sound files, and restores theses permissions on subsequent runs, so that the user only has to grant permission once. But even that might not be necessary, depending on where we store the file.

cspiegel commented 1 month ago

Alright, I think what I'll do is overengineer it for the first release. I'll support both runtime setting of the location via an environment variable and compile-time setting as well. It can be refined in further releases as necessary. This is roughly a "private API", so to speak, so it doesn't need to be preserved from release to release. Since Bocfel is included with Spatterlight/Gargoyle, we're capable of updating how we set the patches file when we import newer versions, if we determine changes are necessary.

I'll get a new release out shortly.

angstsmurf commented 1 month ago

Just to be clear, Spatterlight is not distributed as a DMG disk image, and I can't think of a situation where anyone would want to run it from a mounted DMG.

The macOS version Gargoyle is, however, and I do think that Gargoyle has actually never worked properly when run directly from the mounted DMG. That is a different matter, though.

cspiegel commented 1 month ago

Ah, got it; I'm not familiar with the details how how Mac software really works. Gargoyle's build scripts have existed since before I came on board so I've basically just blindly followed them to get things working.

Alright, well, whichever way works best for Spatterlight, you'll have options! And of course, if things are problematic, I'm happy to tweak how it works in a point release.

cspiegel commented 1 month ago

Alright, I've just published Bocfel 2.2. Either define, at build time, the macro ZTERP_PATCH_FILE with a string value that points to a patch file, or set the BOCFEL_PATCH_FILE environment variable at runtime.

This also has pretty decent support for Infocom's graphical games, so if you'd like to give that a smoke test with Spatterlight, that'd be great. I've done most testing with Gargoyle, and a bit of Windows Glk. If there are major issues with it, I'll be happy to try to get them fixed.

cspiegel commented 1 month ago

After some thought, I've come to the conclusion that, for my purposes in Gargoyle, just allowing new patches to be slotted in at compile-time is sufficient. Unless you're going to be releasing patches in between Spatterlight/Gargoyle releases, then there's really nothing to be gained by a dynamic patchfile. That's a legitimate use-case of course, but not one I'm ready to do any work on in Gargoyle at the moment.

So, I think I'll probably, in short order, do a 2.2.1 release with the ability to include a custom set of patches at compile time. This still allows Spatterlight/Gargoyle to include patches in between Bocfel releases, but gets rid of the annoyance of dealing with yet another run-time file.

Point being, if that's enough for you--at least for the time being--I'd recommend not putting any effort into the 2.2 patchfile system.

cspiegel commented 3 weeks ago

I just released Bocfel 2.2.1 with support for a compile-time patch file. At build time, define ZTERP_STATIC_PATCH_FILE to be a filename (in quotes) containing the patch entries. That file will be directly #included into the patches array. The convert program in https://github.com/garglk/zmachine-patches will generate such an include file with --mode bocfel-compiletime, but for simplicity, I'll paste it here:

{
    "Robot Finds Kitten", "130320", 7, 0x4a18,
    {
        {
            0x4912, 8,
            {0xe4, 0x94, 0x05, 0x00, 0x0a, 0x12, 0x5a, 0x05},
            {0xf6, 0x53, 0x01, 0x0a, 0x12, 0x5a, 0x05, 0xb4},
        },
    },
},
{
    "Transporter", "960729", 1, 0x1ac6,
    {
        {
            0x4bd1, 28,
            {0x41, 0x01, 0x00, 0x00, 0x03, 0xb1, 0x52, 0x01, 0x01, 0x03, 0x52, 0x01, 0x01, 0x00, 0x2d, 0xff, 0x00, 0xa0, 0xff, 0xc5, 0xa4, 0xff, 0xff, 0xe8, 0xbf, 0xff, 0x57, 0x00},
            {0x42, 0x01, 0x01, 0x80, 0x09, 0xc3, 0x8f, 0x01, 0x02, 0x31, 0x00, 0x03, 0xb1, 0x52, 0x01, 0x01, 0x03, 0x2d, 0xff, 0x03, 0xa0, 0xff, 0xc5, 0xa4, 0xff, 0xff, 0x57, 0xff},
        },
    },
},