-
Notifications
You must be signed in to change notification settings - Fork 15.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bump target frameworks of C# programs from netcoreapp1.0 to netcoreapp2.1 #5838
Bump target frameworks of C# programs from netcoreapp1.0 to netcoreapp2.1 #5838
Conversation
Move global.json up to root of repo, change SDK ver to 2.2.100 Change .net core sdk in dockerfile for kokoro to ver 2.2.100
@acozzette can you run a kokoro build to make sure this works? |
Sure, I just added the tag to start the build. |
csharp/compatibility_tests/v3.0.0/src/Google.Protobuf.Test/Google.Protobuf.Test.csproj
Outdated
Show resolved
Hide resolved
This will need a run of https://github.com/protocolbuffers/protobuf/blob/c3340b20a8dbf230ddbc68096ea1e8631317d528/kokoro/linux/dockerfile/push_testing_images.sh to make sure the updated docker images are available. |
I pushed a few more changes to this PR - basically introducing a separate Dockerfile for C# tests (which seems to be in alignment with what protobuf team has been striving to do) - I was getting errors when re-building the multi-language Dockerfile |
@anandolee @TeBoring can you give me push access to https://hub.docker.com/u/protobuftesting or build and push the C# docker image from this PR yourself? (my dockerhub ID is https://hub.docker.com/u/jtattermusch) |
@@ -316,7 +316,7 @@ conformance-java-lite: javac_middleman_lite | |||
conformance-csharp: $(other_language_protoc_outputs) | |||
@echo "Writing shortcut script conformance-csharp..." | |||
@echo '#! /bin/sh' > conformance-csharp | |||
@echo 'dotnet ../csharp/src/Google.Protobuf.Conformance/bin/Release/netcoreapp1.0/Google.Protobuf.Conformance.dll "$$@"' >> conformance-csharp | |||
@echo 'dotnet ../csharp/src/Google.Protobuf.Conformance/bin/Release/netcoreapp2.1/Google.Protobuf.Conformance.dll "$$@"' >> conformance-csharp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we call the dll directly instead of dotnet run --no-build
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally feel more confident that this will really only run the pre-built dll as opposed to to running a dotnet command that can potentially do something extra. No reason to change this in this PR.
I pushed the new testing image for C#, restarting tests. |
The new docker image seems to be working fine, but the c# compatibility test is now failing with
|
@@ -0,0 +1,36 @@ | |||
FROM debian:stretch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original was debian:latest, why change it to stretch? Could this cause the issues we're seeing with compatibility testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently latest = stretch, but meaning of debian:latest can change over time, and the idea of the docker image is to provide a stable runtime environment, so pinning to a specific version seems as a logical choice.
In the past, when debian:lastest changed from jessie to streche, that broke many of our dockerfiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only difference I can see between the last successful run I can find (22 days ago) and this one is that the base image was updated from Debian 9.7 to 9.8 but I don't know if that's the issue (I highly doubt it is).
We could just change the script to use path variables rather than copying files. I believe it'd easier to understand and manage that way in the long run.
Try the tests one more time @jtattermusch |
918d198
to
5e3e9fb
Compare
the required dotnet SDK was not available on Kokoro Windows workers, so I added a script to install the right version of SDK before starting a windows build. |
I think the PR is ready for review and merging now. |
Moved global.json up to root of repo, changed SDK version to 2.2.100.
Changed .NET Core SDK in dockerfile for kokoro to version 2.2.100.
.NET Core 1.0 is reaching end of life, it's hard to install, and it's getting in the way of everyone's fun.
This should fix issues with #5802