Skip to content

[msbuild] Do not allow iOS and Mac to share the same Xamarin.MacDev* … - #2875

Closed
jstedfast wants to merge 4 commits into
masterfrom
msbuild-keep-em-separated
Closed

[msbuild] Do not allow iOS and Mac to share the same Xamarin.MacDev* …#2875
jstedfast wants to merge 4 commits into
masterfrom
msbuild-keep-em-separated

Conversation

@jstedfast

Copy link
Copy Markdown
Member

@jstedfast jstedfast added the do-not-merge Do not merge this pull request label Oct 10, 2017
@jstedfast

Copy link
Copy Markdown
Member Author

@rolfbjarne I need your help with fixing up the Makefile to correctly copy the Xamarin.iOSDev.Tasks* assemblies from their proper locations.

The problem is that the Makefile rules use the assembly name to construct the path, e.g.:

Xamarin.iOSDev.Tasks -> Xamarin.iOSDev.Tasks/bin/Release/Xamarin.iOSDev.Tasks.dll

but this assembly is actually located in Xamarin.MacDev.Tasks/bin/Release/Xamarin.iOSDev.Tasks.dll

@monojenkins

Copy link
Copy Markdown
Contributor

Build failure

@rolfbjarne rolfbjarne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crazy idea here: could it be easier to just include everything into Xamarin.iOS.Tasks[.Core].dll?

@jstedfast

Copy link
Copy Markdown
Member Author

@rolfbjarne I thought of that, but that does make sharing code between Mac & iOS a bit harder...

@rolfbjarne

Copy link
Copy Markdown
Member

@jstedfast why? can't you just link to the source files in the corresponding projects?

@monojenkins

Copy link
Copy Markdown
Contributor

Build failure

@jstedfast

Copy link
Copy Markdown
Member Author

iOS.Tasks.Core subclasses MyTaskBase and calls it MyTaskBase and then iOS.Tasks subclasses again and calls it MyTask

@jstedfast

Copy link
Copy Markdown
Member Author

Yes, this is all safe syntactically from a namespace point of view all in the same assembly, but it's still awkward

@jstedfast

Copy link
Copy Markdown
Member Author

anyway, I found a way to make fix the Makefile in a simple way, so all works now :)

@jstedfast jstedfast removed the do-not-merge Do not merge this pull request label Oct 12, 2017

@kzu kzu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not the way simpler approach of just renaming the assembly when referencing the project instead?

This is the approach we use with Cecil: https://github.com/xamarin/XamarinVS/blob/master/External/Mono.Cecil.overrides

You should be able to perform that renaming automatically in the ProjectReference itself:

<ProjectReference ...>
  <AdditionalProperties>AssemblyName=...</AdditionalProperties>
</ProjectReference>

That way, the entire build/reference/copy would just work. Seems like maintaining stuff in sync would be more work for little gain, unless I'm missing some other requirement

@monojenkins

Copy link
Copy Markdown
Contributor

Build failure

@jstedfast

Copy link
Copy Markdown
Member Author

I don't understand the Cecil approach or how that could/would work.

@spouliot spouliot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging into a single assembly per-platform also sounds nice to my ears.

There's always some chance that Apple could break the simultaneous release of iOS/tvOS/watchOS so having less shared binaries help if we ever need to split things.

Comment thread msbuild/Makefile
.PHONY: $(MSBUILD_SYMLINKS)

# Xamarin.MacDev.dll comes from the Xamarin.MacDev repository
# Newtonsoft.Json.dll comes from a NuGet

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide QA a list of tasks that required Newtonsoft.Json.dll so they can make sure the changes are tested correctly ?

<Reference Include="System.Xml" />
<Reference Include="Mono.Cecil, Version=0.9.5.0, Culture=neutral, PublicKeyToken=0738eb9f132ed756">
<Private>True</Private>
<HintPath Condition=" Exists('..\..\packages\Mono.Cecil.0.9.5.0\lib\net40\Mono.Cecil.dll') ">..\..\packages\Mono.Cecil.0.9.5.0\lib\net40\Mono.Cecil.dll</HintPath>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's also an assembly that can change between versions
and can include breaking changes (it's happening with mono 5.8)

@monojenkins

Copy link
Copy Markdown
Contributor

Build failure

@kzu

kzu commented Nov 1, 2017

Copy link
Copy Markdown
Contributor

Attached is the basic idea @jstedfast with three fake projects:

  1. MacDev (the one upstream in Xamarin.MacDev), the one you want to reuse the code but not the assembly name
  2. Two tasks projects, MacDev.Tasks and iOSDev.Tasks which both reference 1.

Both tasks project reference the MacDev one like so:

    <ProjectReference Include="..\Xamarin.MacDev\Xamarin.MacDev.csproj">
      <Project>{46e80364-c523-4b95-8c2b-ac1f1d047a15}</Project>
      <AdditionalProperties>AssemblyName=Xamarin.iOSDev;OutputPath=$(MSBuildProjectDirectory)$(OutputPath)</AdditionalProperties>
      <Name>Xamarin.MacDev</Name>
    </ProjectReference>

Passing the desired AssemblyName as an additional property to pass to the project reference when building it. The MacDev project in turn just needs to honor those overrides when present:

    <AssemblyName Condition="'$(AssemblyName)' == ''">Xamarin.MacDev</AssemblyName>
    ...
    <OutputPath Condition="'$(Condition)' == ''">bin\Release\</OutputPath>

This means you don't need to maintain duplicate projects at all. The end result is the same you have achieved by duplicating all projects and modifying build script: each tasks project ends up with its own distinct copy of a uniquely named assembly for its own tasks. Gotta love MSBuild ;)

Xamarin.MacDev.zip

@kzu

kzu commented Nov 1, 2017

Copy link
Copy Markdown
Contributor

I have to say though, that using the ILRepack nuget package and a bit of MSBuild sounds even better, simpler and more fool proof.

@jstedfast

Copy link
Copy Markdown
Member Author

ILRepack is licensed under the Apache 2.0 license and is not MIT, is that something we can use?

@jstedfast

Copy link
Copy Markdown
Member Author

This PR has been replaced by #3007

@jstedfast jstedfast closed this Nov 15, 2017
@jstedfast
jstedfast deleted the msbuild-keep-em-separated branch November 15, 2017 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants