Move to .NET Standard 1.6 and dotnet CLI #42

Helcaraxan · 2019-08-05T17:30:17Z

This PR aims to eliminate a few of the issues that I have been encountering in the regeneration of the SDK and its associated scripts.

Highlights:

Move from using a bastardised / mixed version of .NET Framework 4.5.1 with .NET Standard 1.5 to using .NET Standard 1.6. This could technically be considered as a breaking change although the current situation is not very clear on requirements: .NET Standard 1.5 is only compatible with .NET Framework 4.6.1 and thus our current scheme is undefined.
Use of dotnet CLI instead of msbuild as it nicely wraps functionality of both msbuild and nuget and has cleaner CLI interfaces.
Update of the used docker container to a more recent mainline image (and LTS).

RNabel

First pass

RNabel · 2019-08-06T13:27:20Z

Dockerfile

-ENV JAVA_HOME /usr/lib/jvm/java-8-openjdk-amd64/
-RUN export JAVA_HOME
+# Adapted from: https://github.com/dotnet/dotnet-docker/blob/master/2.2/sdk/bionic/amd64/Dockerfile
+FROM buildpack-deps:bionic-scm


What's the diff between the two Dockerfiles? Could you instead use FROM mcr.microsoft.com/dotnet/core/sdk:2.2 and add whatever needs to change? This will also reduce container build time.

The difference is that the one we were using, as well as the 2.2 you are suggesting, are based on Debian 9 (which is currently the stable -1 version). This new one is the official recommended one for Ubuntu 18.04, a.k.a bionic. Given our overall reliance on Ubuntu over Debian this seems like a good move to improve reproducibility between CI builds and the local development experience.

The docker container build time is 1m45 - 2m. This included about ~30/45 seconds of building and packing the Platform SDK for use by the subsequent scenarios. This stands against an average 12/15 minutes of scenario runtime. From that perspective I'd happily take the extra time-hit for the improved convergence between environments, given that the increase in container build time is less than the usual variability between builds of the scenario test times.

If you want the bionic base, use the tags 2.2.401-bionic, or 2.2-bionic (the latter being less specific). See full tag list here: https://hub.docker.com/_/microsoft-dotnet-core-sdk/

Since there's a tag I strongly prefer using it. I don't want to maintain this mess :D

I wholeheartedly agree. 😆 Thanks for finding those links had gone crazy searching for them myself.

RNabel · 2019-08-06T13:27:58Z

Dockerfile

+# Build the examples for use by scripts.
 ARG SDK_VERSION
-RUN msbuild examples/examples.sln /p:Configuration=Release /p:Version=$SDK_VERSION /t:Clean,Build -verbosity:minimal
+RUN dotnet build examples/examples.sln --configuration Release -p:Version=$SDK_VERSION


What's the longform of -p? --parameter?

It's basically taken over from msbuild and is not even properly documented on the dotnet build help. Just look at the very end of the page for the sole mention (or search -p). Hence I'll keep it as is.

RNabel · 2019-08-06T13:28:34Z

apis/apis.csproj


    <PropertyGroup>
+        <Authors>Improbable</Authors>
+        <Copyright>Copyright 2018 Improbable</Copyright>


2019? Or where does this come from?

Will update but this was just a reorder of existing lines.

RNabel · 2019-08-06T13:31:15Z

scripts/release.sh


 set -e -u -o pipefail

 REPO_ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)


Maybe:

REPO_ROOT=$(realpath "${BASH_SOURCE[0]}")/..")

Not sure what the macOS compatibility is here though.

Not going to move this ATM. This PR does not aim to make the script perfect in its entirety. It focuses more on the dotnet migration. Later iterations will improve it further.

RNabel · 2019-08-06T13:33:00Z

scripts/release.sh

+dotnet pack apis/apis.csproj --no-build --configuration Release -p:"PackageVersion=${SDK_VERSION}"
+
+pushd "${BIN_DIR}/${TARGET_FRAMEWORK}"
+zip -r "${PACKAGE_DIR}/${SDK_VERSION}.zip" ./*


This will miss all files/dirs at the top-level AND starting with a . - probably not a problem just want to flag it.

This is how the script works at the moment. It's not a problem. And a candidate for future improvement.

RNabel · 2019-08-06T13:34:09Z

scripts/release.sh

-nuget push "${ARTEFACT_DIR}/Improbable.SpatialOS.Platform.${SDK_VERSION}.nupkg" -Source https://api.nuget.org/v3/index.json
+dotnet nuget push "${PACKAGE_DIR}/Improbable.SpatialOS.Platform.${SDK_VERSION}.nupkg" \
+  --source https://api.nuget.org/v3/index.json \
+  --api-key "${NUGET_API_KEY}"


If the key is in the environment it will be accessible to basically all engineers (in the build logs). Can dotnet take a file?

This script is currently run manually and you need to get if from LastPass. This will be moved to Vault when we automate this process (Soon ™️ )

Helcaraxan requested review from RNabel and dgavedissian August 5, 2019 17:30

Helcaraxan self-assigned this Aug 5, 2019

improbable-prow-robot added jira/no-ticket Indicates a PR has no corresponding JIRA ticket size/M Denotes a PR that changes 40-149 lines, ignoring generated files. labels Aug 5, 2019

Helcaraxan force-pushed the consolidate-scripts branch 3 times, most recently from 272504a to de0feb8 Compare August 5, 2019 18:19

improbable-prow-robot added size/L Denotes a PR that changes 150-299 lines, ignoring generated files. and removed size/M Denotes a PR that changes 40-149 lines, ignoring generated files. labels Aug 5, 2019

Helcaraxan mentioned this pull request Aug 6, 2019

Hand over CI pipeline control #39

Merged

Helcaraxan force-pushed the consolidate-scripts branch from de0feb8 to 1418aa4 Compare August 6, 2019 13:24

RNabel reviewed Aug 6, 2019

View reviewed changes

Helcaraxan force-pushed the consolidate-scripts branch from 1418aa4 to 1c1df0a Compare August 6, 2019 17:22

Move to .NET Standard 1.6 and dotnet CLI

e058320

Helcaraxan force-pushed the consolidate-scripts branch from 1c1df0a to e058320 Compare August 7, 2019 07:52

improbable-prow-robot added size/M Denotes a PR that changes 40-149 lines, ignoring generated files. and removed size/L Denotes a PR that changes 150-299 lines, ignoring generated files. labels Aug 7, 2019

Helcaraxan requested a review from RNabel August 7, 2019 10:22

RNabel approved these changes Aug 7, 2019

View reviewed changes

Helcaraxan merged commit 363f668 into master Aug 7, 2019

Helcaraxan deleted the consolidate-scripts branch August 7, 2019 14:44


		set -e -u -o pipefail

		REPO_ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)

Move to .NET Standard 1.6 and dotnet CLI #42

Move to .NET Standard 1.6 and dotnet CLI #42

Uh oh!

Conversation

Helcaraxan commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RNabel 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Helcaraxan commented Aug 5, 2019 •

edited

Loading