Skip to content

Conversation

@xmcclure
Copy link
Contributor

@xmcclure xmcclure commented Feb 8, 2017

To be merged after Wrench testing is complete and after the next xamarin-android branch.

@jonpryor jonpryor added do-not-merge PR should not be merged. enhancement Proposed change to current functionality. labels Feb 8, 2017
<ItemGroup>
<MonoDocCopyItem Include="monodoc.dll" />
<MonoDocCopyItem Include="monodoc.dll.mdb" />
<MonoDocCopyItem Include="monodoc.pdb" Condition="Exists ('monodoc.pdb')" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this Condition can possibly work. It's going to use the current working directory, and there are no odds that the current working directory will be external/mono/mcs/class/lib/net_4_x.

I think what you'd actually want to do is remove the Condition on this line and the following line, and instead update @(_MonoDocCopyItems) -- line 30? -- to be:

<ItemGroup>
  <_MonoDocCopyItems
      Condition=" Exists ('$(_MonoOutputDir)\%(Identity)') "
      Include="@(MonoDocCopyItem->'$(_MonoOutputDir)\%(Identity)')"
  />
  <_MonoDocInstalledItems
      Condition=" Exists ('$(_MonoOutputDir)\%(Identity)') "
      Include="@(MonoDocCopyItem->'$(_MandroidDir)\%(Identity)')"
  />
</ItemGroup>

@dellis1972: Does the above sound correct to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

This won't actually work either; top-level <ItemGroup/> values are processed at file-load time, i.e. when mono-runtimes.targets is processed, and thus is quite possible that Mono won't have been built yet, and thus the Condition expressions here will still fail.

I think we'll need a "helper" target?

Copy link
Contributor

@dellis1972 dellis1972 Feb 9, 2017

Choose a reason for hiding this comment

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

@xmcclure sorry, I completely forgot that we needed the path in there.

Best thing to do is to write a target which will collect the files that exist. You can use the ItemGroup code that @jonpryor mentioned and pop that in the new Target. That will add the items to the ItemGroup at run time not load time.

<Target Name="_CollectDebuggingFiles">
<ItemGroup>
  <_MonoDocCopyItems
      Condition=" Exists ('$(_MonoOutputDir)\%(Identity)') "
      Include="@(MonoDocCopyItem->'$(_MonoOutputDir)\%(Identity)')"
  />
  <_MonoDocInstalledItems
      Condition=" Exists ('$(_MonoOutputDir)\%(Identity)') "
      Include="@(MonoDocCopyItem->'$(_MandroidDir)\%(Identity)')"
  />
</ItemGroup>
</Target>

Then just add the _CollectDebuggingFiles target to the DependsOnTargets list of the target you need it to run before.

@monojenkins
Copy link

Hello! I'm the build bot for the Mono project.

I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done.

Contributors can ignore this message.

1 similar comment
@monojenkins
Copy link

Hello! I'm the build bot for the Mono project.

I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done.

Contributors can ignore this message.

@jonpryor
Copy link
Contributor

jonpryor commented Feb 9, 2017

Additionally, you need to update @(BundleItem), in GetMonoBundleItems (around line 479). If you go with the _CollectDebuggingFiles approach, then _CollectDebuggingFiles needs to be added to DependsOnTargets for GetMonoBundleItems (line 477). If you opt to treat debug symbol files separately, you'll need to separately update @(BundleItem).

@xmcclure
Copy link
Contributor Author

xmcclure commented Feb 9, 2017

Should the existing _InstallMonoSymbolicate target depend on anything?

The previous commit's attempts to copy the pdb/mdb files for monodoc had a problem where they would be evaluated too early (probably before the files existed) and in the wrong directory. To address this, the files are instead specified as a MonoDocCopyItemOptional, a new target _GetMonodocItems is created and the ItemGroups for _MonoDocCopyItems/_MonoDocInstalledItems are evaluated in this target with correct Exists logic for the Optional files. Additional DependsOnTargets attributes were distributed through the file to ensure this new target is evaluated at the correct time.
@xmcclure
Copy link
Contributor Author

xmcclure commented Feb 9, 2017

Okay, I think this is correct (please see full commit notes for details of what I did), and after doing a git clean xffd and rebuild the pdb appears to be getting copied to the right directory...

andi:xamarin-android andi$ find ./* | grep pdb | grep -i monodoc
./bin/Debug/lib/mandroid/monodoc.pdb
./external/mono/mcs/class/lib/net_4_x/monodoc.pdb

Note, I had to add a bunch of DependsOnTargets-es to make this all work right. I did not touch the Depends logic for anything not specifically related to my changes, however, and I think you need to audit the dependencies of all items in ForceBuildDependsOn. Your current logic where you assume that separating X;Y by semicolons is enough to ensure X executes before Y is not safe, and (apparently!) can get subverted merely because a DependsOnTargets was added in a different part of the file.

Include="@(MonoDocCopyItemOptional->'$(_MonoOutputDir)\%(Identity)')"
/>
</ItemGroup>
<ItemGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

We can collapse these into one ItemGroup. there is not need to separate them. unless @jonpryor said to :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're not the same, one contains "mdoc.exe" and the other doesn't

Copy link
Contributor

Choose a reason for hiding this comment

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

You can mix different items within an ItemGroup so

<ItemGroup>
    <Foo Include="Somefile.dll" />
    <Bar Include="Someotherbarfile.dll" />
</ItemGroup>

MSbuild and xbuild will evaluate each "item" separately. But its sometimes useful to put items together in logical groups just to make it easier to maintain. This is probably one of those cases :)

@xmcclure
Copy link
Contributor Author

Moved to PR #445

@xmcclure xmcclure closed this Feb 15, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

do-not-merge PR should not be merged. enhancement Proposed change to current functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants