Add cross IBCMerge #33282

DrewScoggins · 2018-11-06T21:51:52Z

Currently we cannot apply IBC data on Linux so we need to apply the data
on Windows. This script will handle the application of the Linux IBC
data on Windows.

Currently we cannot apply IBC data on Linux so we need to apply the data on Windows. This script will handle the application of the Linux IBC data on Windows.

DrewScoggins · 2018-11-06T21:52:06Z

PTAL @adiaaida @maririos

michellemcdaniel

Are we sure there isn't a better place than the root of the repo to put this? Maybe it can be moved to the eng directory, even though it isn't strictly arcade related?

michellemcdaniel · 2018-11-06T22:12:35Z

crossIBC.ps1

+    echo $count
+}
+
+.\build.cmd -restore /p:OptionalToolSource=$ToolSource /p:OptionalToolSourceUser=$ToolUser /p:OptionalToolSourcePassword=$ToolPAT /p:EnableProfileGuidedOptimization=true /p:IBCTarget=Linux -release -ci


How/where is IBCTarget used?

This was added in, dotnet/buildtools#2196. This allows us to specify whether we are pulling down Linux or Windows IBC data

maririos · 2018-11-06T22:50:28Z

crossIBC.ps1

+            Copy-Item $fileToOptimize -Destination $copyLocation
+        }
+    }
+    echo $count


What does $count represent?

This was just debug stuff that I forgot to remove. I will take this out.

crossIBC.ps1

DrewScoggins · 2018-11-07T17:59:19Z

To speak to @adiaaida earlier comment. I am fine putting this in another spot, I don't believe it is necessary for this to be in the root. I was just not sure where it should go. I would be fine putting it in the eng directory, but want to make sure that would not be an issue with Arcade.

ericstj · 2018-11-07T23:02:17Z

but want to make sure that would not be an issue with Arcade.

Nope, we're putting other things in there.

Can you be sure to add some comments to the script explaining what it does or link to documentation in it?

DrewScoggins · 2018-11-07T23:05:14Z

Yes, of course. I will comment this up so that it is clear what it is for. I will also move this over into the eng directory.

ericstj · 2018-11-07T23:12:21Z

crossIBC.ps1

+    $IBCMergePath = gci -recurse .\.packages | where {$_.Name.Contains("ibcmerge.exe")}
+    if(!$IBCMergePath)
+    {
+        $IBCMergePath = gci -recurse C:\microsoft.dotnet.ibcmerge | where {$_.Name.Contains("ibcmerge.exe")}


Do you really need to search the C: drive?

There is currently a bug in the restore optional tooling step that is causing all of the optional tooling to be restored to the root of the drive. That is why I am searching here.

We should fix that bug rather than depend on it. I hit a similar bug a couple days ago and fixed it: 2e3b5eb#diff-4b2f402501d23abbede6eac0f47783e4R70.

Where's the restore optional tooling step implemented? Sounds like its using the wrong property for the packages folder.

Optional tools are restored here: https://github.com/dotnet/buildtools/blob/0fa455c550abb4f49f4830a7f2fa78609f373fd0/src/Microsoft.DotNet.Build.Tasks/PackageFiles/OptionalTooling.targets

I did some investigation into this and was unable to figure out why it was restoring to the root. The package dir that was getting set was correct, but when it actually did the restore step it would not write them there.

Hmm, when I try and repro this it is now restoring into the .packages directory. I will take out looking for the tool in the root.

ericstj · 2018-11-07T23:13:23Z

Curious why this even needs to be a separate script, why not use a build target and do this in msbuild?

DrewScoggins · 2018-11-08T18:57:00Z

I started with trying to do this via MSBuild, but ran into issues since I was trying to apply the data from Linux on Windows. I imagine that I could figure this out, but it was much easier to use the powershell script.

DrewScoggins · 2018-11-13T18:09:17Z

Moved the script over to the eng directory and addressed other points of feedback.

maririos · 2018-11-13T19:12:32Z

eng/crossIBC.ps1

+        {
+            if(!$UsePartialNgen)
+            {
+                $count = $count + 1


we shouldn't need this anymore

Oops. I'll remove this.

michellemcdaniel · 2018-11-13T22:27:51Z

eng/crossIBC.ps1

+    }
+}
+
+cd ..


Why are we changing up a directory?

Because the script was meant to run from the root of the repo and we are in the eng dir. I could add a parameter that we could pass in that is the full path to the root of the directory as well.

Better to be more specific about directories in case one day the structure of the repo changes

This should be taken care of

michellemcdaniel

LGTM!

maririos

Make sure to delete $count = $count + 1 before merging

* Add cross IBCMerge Currently we cannot apply IBC data on Linux so we need to apply the data on Windows. This script will handle the application of the Linux IBC data on Windows. * Address feedback * Move script to eng dir * Add path parameter * Remove debug code

* Add cross IBCMerge Currently we cannot apply IBC data on Linux so we need to apply the data on Windows. This script will handle the application of the Linux IBC data on Windows. * Address feedback * Move script to eng dir * Add path parameter * Remove debug code Commit migrated from dotnet/corefx@e0cb81d

Add cross IBCMerge

634fb0b

Currently we cannot apply IBC data on Linux so we need to apply the data on Windows. This script will handle the application of the Linux IBC data on Windows.

michellemcdaniel reviewed Nov 6, 2018

View reviewed changes

maririos requested a review from weshaggard November 6, 2018 22:48

maririos reviewed Nov 6, 2018

View reviewed changes

crossIBC.ps1 Outdated Show resolved Hide resolved

maririos reviewed Nov 6, 2018

View reviewed changes

crossIBC.ps1 Outdated Show resolved Hide resolved

ericstj reviewed Nov 7, 2018

View reviewed changes

DrewScoggins added 2 commits November 13, 2018 10:06

Address feedback

4b0dd0a

Move script to eng dir

c45d1a9

maririos reviewed Nov 13, 2018

View reviewed changes

michellemcdaniel reviewed Nov 13, 2018

View reviewed changes

Add path parameter

06561f7

michellemcdaniel approved these changes Nov 14, 2018

View reviewed changes

maririos approved these changes Nov 14, 2018

View reviewed changes

Remove debug code

2aeab52

DrewScoggins merged commit e0cb81d into dotnet:master Nov 15, 2018

karelz added this to the 3.0 milestone Nov 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cross IBCMerge #33282

Add cross IBCMerge #33282

DrewScoggins commented Nov 6, 2018

DrewScoggins commented Nov 6, 2018

michellemcdaniel left a comment

michellemcdaniel Nov 6, 2018

DrewScoggins Nov 7, 2018

maririos Nov 6, 2018

DrewScoggins Nov 7, 2018

DrewScoggins commented Nov 7, 2018

ericstj commented Nov 7, 2018

DrewScoggins commented Nov 7, 2018 •

edited

Loading

ericstj Nov 7, 2018

DrewScoggins Nov 8, 2018

ericstj Nov 8, 2018

michellemcdaniel Nov 8, 2018

DrewScoggins Nov 8, 2018

DrewScoggins Nov 8, 2018

ericstj commented Nov 7, 2018

DrewScoggins commented Nov 8, 2018

DrewScoggins commented Nov 13, 2018

maririos Nov 13, 2018

DrewScoggins Nov 13, 2018

michellemcdaniel Nov 13, 2018

DrewScoggins Nov 13, 2018

maririos Nov 14, 2018

michellemcdaniel Nov 14, 2018

DrewScoggins Nov 14, 2018

michellemcdaniel left a comment

maririos left a comment

+                  }
+              }
+              cd ..

Add cross IBCMerge #33282

Add cross IBCMerge #33282

Conversation

DrewScoggins commented Nov 6, 2018

DrewScoggins commented Nov 6, 2018

michellemcdaniel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrewScoggins commented Nov 7, 2018

ericstj commented Nov 7, 2018

DrewScoggins commented Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj commented Nov 7, 2018

DrewScoggins commented Nov 8, 2018

DrewScoggins commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michellemcdaniel left a comment

Choose a reason for hiding this comment

maririos left a comment

Choose a reason for hiding this comment

DrewScoggins commented Nov 7, 2018 •

edited

Loading