[Mono-osx] [MonoMac] patch for MM-addin, Existing Framework

[Michael Hutchinson]

On Wed, Jan 26, 2011 at 10:01 AM, Duane Wandless <duane at wandless.net> wrote:
> Thanks, here is another update based on feedback.
> Changes:
> * renamed the menu item
> * save to the project file as MSbuild items
> * use CodeDOM to generate code
> * generates MonoMacFrameworks.Initialize() in MonoMacFrameworks.cs
> * saves to project whether relative or absolute path
> * relies on MonoMac exporting dlopen and dlerror in
> the MonoMac.ObjCRuntime.Dlfcn class
> Issues:
> * Project pad does not update when frameworks are added/deleted
> * build must always be performed since build process modifies
> MonoMacFrameworks.cs in wrong step
> * UI needs more work

Great, this is definitely moving quickly in the right direction :)

A few more comments:
* your patch is missing the added files - it only has the modified files.
* please move the CodeDom code into a separate method
* you should update the generated file before the base.OnBuild call,
so it happens before the compilation
* it might be nice to update the generated code immediately after any
frameworks are added/removed. Not a must-have though.
* you should only copy the frameworks if they change
* the generated file should only be updated at build time if the
project file is newer than the generated file. GetNeedsBuilding should
check this too.
* don't hardcode CSharpCodeProvider, you can get the appropriate
language-specific CodeDomProvider from the DotNetProject.
* please use var when the type is obvious from the assignment,
especially newing, e.g. var foo = new FooBar (...);
* property initializers can be used to make CodeDom a bit nicer.
* if (Directory.Exists (destFramework) == true) should be if
(Directory.Exists (destFramework))
* wouldn't it be better to throw instead of writing a console message,
since missing frameworks will almost certainly break an app?
* partial modifiers are no longer needed
* string paths can be implicitly cast to/from MD's FilePath struct,
which neatens up path manipulation a bit
* should there be a distinction for "local copy" frameworks, like dll
references have? In both cases you need the loading code, but only for
local copy does the framework need to be copied into the app AFAIK.

Re. Miguel's comments, it would be good to check for missing
frameworks when copying them and return errors in a BuildResult
instead of letting MD present the IO exception directly.

I'm not sure about the symlinking, since the apps we build are
currently distributable without additional packaging steps, and
symlinking would break that. Frameworks won't really hurt build
iteration times if we fix it to to only copy frameworks when they
change.

-- 
Michael Hutchinson
http://mjhutchinson.com