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

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

If you _really_ want to keep it as one patch, then I'm gonna have to
spend more time reviewing your the full set of changes and asking more
questions.

For one, I don't like the renaming of main.m in PrefsEditor. It seems
it poses a restriction on how things are named which can cause subtle
problems.

Perhaps it would be better have a convention of xyz.ext.o - for
example xyz.cpp.o or xyz.m.o. I've seen this convention used before in
other projects. This way there's no confusion as to what object file
should come from where.

-Alexei

On Wed, Feb 4, 2009 at 4:43 PM, Alexei Svitkine
<ale...@gm...> wrote:
> 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
>>
>