Re: [B2-devel] Fixes for BasiliskII and SheepShaver for Mac OS X 10.4 and 10.5 ppc and intel

[Alexei S. <ale...@gm...>]

Smaller patches are easier to review because I can look at the code
and at the description of what the patch does and see if it
corresponds. A giant patch of "make things work" is hard to review for
mistakes.

For example:

-	       MacOSX/macos_util_macosx.h MacOSX/extfs_macosx.h \
+	       MacOSX/macos_util_macosx.h Unix/cpr.sh \


Why is extfs_macosx.h removed? Is this intentional or a mistake?

if this was part of a smaller patch that described exactly what's
being added (ex: added cpr.sh to replace cp command), then I would
know for sure its a mistake. But here I don't know - perhaps its
related to your other change.

I also don't buy your argument of not being able to test it on the
other machine. By that logic no other change to the code should be
made after I add your patch because it's impossible to test on all the
machines!

I think that its possible to split the patch into incremental changes
while keeping the end result the same (and thus equivalent to the code
you tested that worked).

Also if one of the changes causes an issue, its easier to roll back
just that change instead of the huge patch...

-Alexei

On Wed, Feb 4, 2009 at 2:21 PM, Mike Sliczniak
<bas...@sp...> wrote:
> Hi,
>
> Good catches.
>
> On Tue, 3 Feb 2009, Alexei Svitkine - ale...@gm... wrote:
>
>> Also I don't see the why you're replace cp -f with cpr.sh. It should
>> only replace cp -R.
>
> Resource forks and extended attrs don't need to be conserved.
>
>> Also you have an "i" for some reason after an #undef:
>>
>> +#undef       MACH_EXCEPTION_CODES                                    i
>
> I use vim, that was a stray insert when I was already in insert. Get rid
> of that i. Luckily cpp does not care about that, but I got lucky.
>
>> > Also from a quick glance, this makes no sense:
>> >
>> >  sigsegv_address_t sigsegv_get_fault_instruction_address(sigsegv_info_t *SIP)
>> >  {
>> >  #ifdef HAVE_MACH_EXCEPTIONS
>> > +#ifdef HAVE_MACH_EXCEPTIONS
>> >        if (!SIP->has_thr_state) {
>> >                mach_get_thread_state(SIP);
>> >
>> >                SIP->pc = (sigsegv_address_t)SIGSEGV_FAULT_INSTRUCTION;
>> >        }
>> >  #endif
>> > +#endif
>> >        return SIP->pc;
>> >  }
>
> That should be #ifdef EMULATED_PPC instead of #ifdef HAVE_MACH_EXCEPTIONS.
> I never tested the sigsegv instruction skipper so that is why I did not
> notice this one. When you are building for a PPC, SIGUSR2 is used for the
> interrrupt trigger and signal handlers instead of the mach exception
> handling. If you do thread_get_state here you get a crash since you are
> getting the state of your own running thread which is not suspended. I
> believe that Gwenole tried to get the signal handlers working together
> with mach exceptions sometime back after I added the mach exception stuff
> to BasiliskII but was not successful and so that is why it is the way it
> is. Also back then I did not understand how SheepShaver (since I never
> needed it) worked so I was no help. One of the things I want to
> investigate is if thread_suspend and mach exception handlers instead of
> signal handlers would be an improvement.
>
> About breaking-up the patches. I would rather not. Also some are in the
> same files, would you like patches that do not patch cleanly or would you
> like me to submit a patch and wait then prepare another? I'm not too
> comfortable with that because like I wrote before, the PPC 10.4 machine I
> had to test and build on has died. I am afraid to perturb too much and
> make a set of patches that do not build or run on 10.4. What I did to
> create this is I made some patches to the code until I got it to compile
> and run on one combination of os and cpu and then I tried it on another. I
> tweaked until it worked on the new one and then I learned that those no
> longer worked on the previous. I had to do that for a few combinations of
> different options for SheepShaver and BasiliskII on three machines. It
> took a lot of trial and error (and time) to get a nice set of patches that
> worked on every combination I had access to.
>
> But here is a description if that helps:
>
> The sdl audio change is easy to see. If you do not want that one, that is
> fine, it only makes things more tolerable on slower machines. (Meaning the
> system beep tends not to break-up.) It is only a change to a single
> file.
>
> The other source code changes are all about getting it to compile under
> Tiger and Leopard.
>
> The cpr.sh was to get .app bundles that worked right on Tiger because if
> they had extra detritus they would not work right. Leopard was much more
> forgiving.
>
> The changes to the autoconf stuff, most of it was to support building with
> a prebuilt SDL framework, it is pretty easy to see what belongs with that.
> Then there is a smidge so that you can have SDL audio if you wish even
> with OS X. The rest are little tweaks to get it to build and link a
> working binary for Tiger and Leopard.
>
> The two new files and the changes to the GUI prefseditor files were for
> the build to continue to work correctly after adding the option of the SDL
> Framework build.
>
> Let me know if you have other concerns especially about the patches to the
> code or want me to do something in a different way like you only want me
> to patch a subset and forget about the rest.
>
> mzs
>
>
> ------------------------------------------------------------------------------
> Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM)
> software. With Adobe AIR, Ajax developers can use existing skills and code to
> build responsive, highly engaging applications that combine the power of local
> resources and data with the reach of the web. Download the Adobe AIR SDK and
> Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com
> _______________________________________________
> basilisk-devel mailing list
> bas...@li...
> https://lists.sourceforge.net/lists/listinfo/basilisk-devel
>