Add a unit test for ARM projects to verify correct files are generated by jeremylindsayni · Pull Request #1251 · dotnet/sdk

jeremylindsayni · 2017-05-24T20:12:56Z

This adds a unit test to cover #1239 - this checks that the correct files are generated for self contained projects which target ARM runtimes.

Quick note for clarity - this request is just a unit test - the original fix was submitted by @eerhardt in pull request #20187.

@eerhardt @gkhanna79 @ericstj

Updated test from Fact to Theory, with RID as parameter

dnfclas · 2017-05-24T20:13:01Z

@jeremylindsayni,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

dnfclas · 2017-05-24T20:18:22Z

@jeremylindsayni, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

dsplaisted · 2017-05-24T21:26:20Z

Thanks @jeremylindsayni!

Since this test depends on a fix in corefx, we'll have to wait for that fix to flow here before the test will pass and we can merge it. The path is that it needs to go into a version of the shared framework, which needs to go into the CLI, and then we need to update the SDK repo to depend on that updated CLI.

eerhardt

Thanks for the PR, @jeremylindsayni. This test looks really good. Just a few comments.

Hopefully we can get a new 2.0 runtime build into this repo soon, so we can merge this test.

eerhardt · 2017-05-24T21:40:44Z

+        [InlineData("win8-arm")]
+        [InlineData("win81-arm")]
+        [InlineData("win10-arm")]
+        public void Publish_standalone_post_netcoreapp2_arm_app(string runtimeIdentifier)


What does post mean in the name?

Hi - this came from a convention in an existing test method that I used as a template earlier in the same class, which was called "Publish_standalone_post_netcoreapp2_app_and_it_should_run".

That method has a comment above the test explaining that distinction between behaviours in pre and post Netcoreapp2.0 (which I guess will be relevant to this test too) - I've copied that in and re-committed the file, hopefully this explains the "post" bit of the method name better?

Sounds good to me. Thanks for the explanation.

eerhardt · 2017-05-24T21:43:34Z

+            var publishDirectory = publishCommand.GetOutputDirectory(
+                targetFramework: targetFramework,
+                runtimeIdentifier: runtimeIdentifier);
+            var selfContainedExecutable = $"Hello{Constants.ExeSuffix}";


{Constants.ExeSuffix} - this won't work correctly when this test is run on OSX or Linux. When running on those platforms, Constants.ExeSuffix will be string.Empty. However, since the project always targets Windows, the output self-contained executable will always be for Windows. Thus no matter which platform this test is run on, the output assembly will be named Hello.exe.

So I think you can just hard-code .exe here.

Yes - I think we spotted this one in parallel, I've changed this one.

eerhardt · 2017-05-24T21:43:48Z

+                runtimeIdentifier: runtimeIdentifier);
+            var selfContainedExecutable = $"Hello{Constants.ExeSuffix}";
+
+            string selfContainedExecutableFullPath = Path.Combine(publishDirectory.FullName, selfContainedExecutable);


This variable doesn't appear to be used.

Yes - just saw this comment too - I'll remove and re-commit.

eerhardt · 2017-05-24T21:44:32Z

+                "Hello.pdb",
+                "Hello.deps.json",
+                "Hello.runtimeconfig.json",
+                $"{FileConstants.DynamicLibPrefix}coreclr{FileConstants.DynamicLibSuffix}",


Same comment as above here. The app will always target Windows. Thus these files will always be named:

coreclr.dll
hostfxr.dll
hostpolicy.dll.

I see what you mean - I've updated the file to have these file names and recommitted the file.

Update the branding in master to preview3.

Edited after code review: - I've fixed a bug where the test expected the generated filename to be Hello on Ubuntu/OSX, which it should actually always be Hello.exe when we're targetting winX-arm; - I've added some comments to the method to explain the naming convention; - I've removed dynamic prefixes/suffixes from "hostfxr.dll", "hostpolicy.dll", "mscorlib.dll" as this test always targets windows.

jeremylindsayni · 2017-05-24T22:44:21Z

Hi @dsplaisted and @eerhardt - thank you for the code review and the comments! I've hopefully addressed these now.

I re-examined the test code after seeing the automated failures on Ubuntu and OSX machines, and I actually now think there was a bug in the test I wrote (which I've now fixed). Specifically when publishing for a windows ARM project on Ubuntu or OSX, the test previously looked for a file named "Hello" when it should always be looking for "Hello.exe" after publishing to a Windows family runtime identifier.

Hopefully this test passes on all the environments after the corefx fix flows across.

eerhardt · 2017-05-25T14:20:21Z

+            // depending on the RuntimeInformation
+            var selfContainedExecutable = "Hello.exe";
+
+            string selfContainedExecutableFullPath = Path.Combine(publishDirectory.FullName, selfContainedExecutable);


The selfContainedExecutableFullPath variable is not used. Can it be removed?

Yes - sorry, I only noticed you asked this earlier, for some reason some comments are collapsed and tagged as outdated, I missed them. I'll update this when I'm in front of my dev machine to remove the unused variable and address any earlier comments I missed.

eerhardt · 2017-05-25T14:21:37Z

+        [InlineData("win-arm")]
+        [InlineData("win8-arm")]
+        [InlineData("win81-arm")]
+        [InlineData("win10-arm")]


What do you think about adding win10-arm64 to this list?

I think that's a good idea, the more we cover the better - the only thing is I don't have an arm64 device to test this on, do we know if the list of files present on arm 32-bit devices should be the same as the 64-bit devices?

Yes, the list of files will be the same. Obviously, the contents of those files will be different, but the names will be the same.

Cool - I'll update that in my next commit.

…variable.

nguerrera · 2017-05-25T19:01:03Z

+            {
+                //  Disabled on full framework MSBuild until CI machines have VS with bundled .NET Core / .NET Standard versions
+                //  See https://github.com/dotnet/sdk/issues/1077
+                return;


IIRC, we have CoreMSBuildOnlyTheory for this now.

nguerrera

Thanks for your contribution!

LOC CHECKIN | sdk-master | 20170529

Merge release/2.0.0 to master

jeremylindsayni · 2017-06-04T17:08:09Z

Hi - should I edit this PR to merge into "dotnet:release/2.0.0" instead of "dotnet:master"?

jeremylindsayni · 2017-06-06T19:56:12Z

Hi @eerhardt - it looks like there's now a pretty complex (and risky looking) merge operation pending on this PR. I think it's safest if I close this PR and try re-applying my test into the current release:2.0.0 branch, what do you think?

eerhardt · 2017-06-06T20:05:28Z

I think it's safest if I close this PR and try re-applying my test into the current release:2.0.0 branch, what do you think?

Yes, I think that is a good strategy. Sorry the branching is getting in the way of progress here. It looks like switching base branches caused a few merge issues.

jeremylindsayni · 2017-06-06T20:19:29Z

Closing this as merge conflicts appeared after switching branch to release:2.0.0, but will re-open after targeting the most up to date code in release:2.0.0.

…203.5 (dotnet#1251) - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.20103.5

jeremylindsayni and others added 4 commits May 23, 2017 21:34

Updated to add a unit test for win10-arm RID

6b15ca8

Update the branding in master to preview3.

eeb1cb1

Updating the version to match the CLI version

da1de1d

Updated test from Fact to Theory

8a6c841

Updated test from Fact to Theory, with RID as parameter

dnfclas added the cla-required label May 24, 2017

dnfclas added cla-signed and removed cla-required labels May 24, 2017

eerhardt approved these changes May 24, 2017

View reviewed changes

jeremylindsayni and others added 3 commits May 24, 2017 22:58

Corrected name of exe to depend on RID

263f671

Merge pull request #1248 from livarcocc/update_branding_in_master

97f1e21

Update the branding in master to preview3.

eerhardt reviewed May 25, 2017

View reviewed changes

eerhardt mentioned this pull request May 25, 2017

Update CLI version to 2.0.0-preview2-006189 #1254

Merged

Modified post review - now testing "win10-arm64", and removed unused …

0f2df3c

…variable.

nguerrera reviewed May 25, 2017

View reviewed changes

nguerrera approved these changes May 25, 2017

View reviewed changes

jeremylindsayni and others added 6 commits May 28, 2017 17:46

Merge branch 'master' into master

43177de

LOC CHECKIN | sdk-master | 20170529

ec90f38

Merge pull request #1264 from v-zbsail/localization

5ad3099

LOC CHECKIN | sdk-master | 20170529

Merge branch 'release/2.0.0' into merge-release

9081105

Merge pull request #1269 from nguerrera/merge-release

a4bceb6

Merge release/2.0.0 to master

Merge branch 'master' into master

1836c90

livarcocc changed the base branch from master to release/2.0.0 June 5, 2017 17:35

jeremylindsayni closed this Jun 6, 2017

jeremylindsayni mentioned this pull request Jun 7, 2017

Add a unit test for ARM projects to verify correct files are generated #1317

Merged

mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020

Update dependencies from https://github.com/dotnet/arcade build 20200…

4858312

…203.5 (dotnet#1251) - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.20103.5

Conversation

jeremylindsayni commented May 24, 2017

Uh oh!

dnfclas commented May 24, 2017

Uh oh!

dnfclas commented May 24, 2017

Uh oh!

dsplaisted commented May 24, 2017

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremylindsayni commented May 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nguerrera left a comment

Choose a reason for hiding this comment

Uh oh!

jeremylindsayni commented Jun 4, 2017

Uh oh!

jeremylindsayni commented Jun 6, 2017

Uh oh!

eerhardt commented Jun 6, 2017

Uh oh!

jeremylindsayni commented Jun 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants