Skip to content
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

[C#] fix net35 with JsonSubTypes #7043

Merged
merged 6 commits into from
Dec 21, 2017
Merged

[C#] fix net35 with JsonSubTypes #7043

merged 6 commits into from
Dec 21, 2017

Conversation

manuc66
Copy link
Contributor

@manuc66 manuc66 commented Nov 23, 2017

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

As mentioned in #6969 this PR update JsonSubTypes to 1.2.0 which is compatible with NET35 without API changes.

As suggested I updated compile.mustache

In order to validate the NET35 compliance I added a new sample generation samples\client\petstore\csharp\SwaggerClientNet35\

Here is the compilation status of sample available in the project:

project build.cmd build status build. sh build status VS2017 build status
csharp\SwaggerClient OK OK OK
csharp\SwaggerClientWithPropertyChanged OK OK OK
csharp\SwaggerClientNet35 NOT OK OK OK
csharp\SwaggerClientNet40 NOT OK OK OK
csharp\SwaggerClientNetCoreProject na na OK
csharp\SwaggerClientNetStandard na na OK

The two NOT OK are not related to this PR nor #6969 but seems to be related to the usage of csc

cc: @mandrean, @jimschubert

@jimschubert
Copy link
Contributor

The change looks good.

What are the errors present in .NET 3.5/4.0 compile scripts? These worked previously (I resolved any issues about a week or two ago), before changing to the nuget package. Are you on an older version if mono, maybe?

I'll verify against this branch when I'm at a computer either tonight or tomorrow and report back with my mono version.

If it's a mono version issue, we'll want to open another pr to check for a minimum version before compilation, so there's no unknowns in build scripts. That's probably a good idea, anyway.

@manuc66
Copy link
Contributor Author

manuc66 commented Nov 23, 2017

@jimschubert I did not try on linux/mono but on windows only.

It looks like the windows scripts are at least missing the langversion configuration. But are those kind of scripts making senses on windows ?

Here are the windows build.log:
SwaggerClientNet35.windows.build.log
SwaggerClientNet40.windows.build.log

@jimschubert
Copy link
Contributor

@manuc66 on Windows, I think it's a little different. If you're on Windows 10, I don't know if you'll be able to install .net 3.5 or 4.0. .net 4.5 is an in place install over .net 4.0, which may compile differently than having only .net 4.0 installed.

I'll need to download the IE developer images again to check.

@manuc66
Copy link
Contributor Author

manuc66 commented Nov 23, 2017

@jimschubert

If I do use msbuild instead of csc on Windows the compilation works:

For net35:

if not exist ".\nuget.exe" powershell -Command "(new-object System.Net.WebClient).DownloadFile('https://dist.nuget.org/win-x86-commandline/latest/nuget.exe', '.\nuget.exe')"
.\nuget.exe restore

%SYSTEMROOT%\Microsoft.NET\Framework\v4.0.30319\msbuild IO.Swagger.sln /p:TargetFramework=3.5

See: https://pastebin.com/ZrCfdbrS

For net40 I still have issue with msbuild that I don't have with VS 2017:

if not exist ".\nuget.exe" powershell -Command "(new-object System.Net.WebClient).DownloadFile('https://dist.nuget.org/win-x86-commandline/latest/nuget.exe', '.\nuget.exe')"
.\nuget.exe restore

%SYSTEMROOT%\Microsoft.NET\Framework\v4.0.30319\msbuild IO.Swagger.sln /p:TargetFramework=4.0

see: https://pastebin.com/5mCLTqn7 (in fact that's only the test library that is failing to build)

@jimschubert
Copy link
Contributor

@manuc66 were you ever able to resolve the issue with .NET 4.0? I intended to test this out but my VM drive crapped out and my main machine didn't have enough space for running a VM. I've upgraded my machine, and could take a look now if you were unable to.

…ure/fix-net35

# Conflicts:
#	modules/swagger-codegen/src/main/resources/aspnetcore/project.json.mustache
@manuc66
Copy link
Contributor Author

manuc66 commented Dec 20, 2017

@jimschubert I did not found a solution for the compilation errors that are linked to the use of csc in the compilation stage.

Note that:

  • the generated code can be build with VS2017
  • the errors generated by csc are not related to the changes made by this PR
  • there is no build error on Linux

@wing328
Copy link
Contributor

wing328 commented Dec 21, 2017

Thanks @manuc66.

We plan to release 2.3.0 today or tomorrow so I hope we can this into master as well.

@wing328
Copy link
Contributor

wing328 commented Dec 21, 2017

I tested build.sh for the following folders and the result is good:

SwaggerClient
SwaggerClientNet40
SwaggerClientNet35
SwaggerClientWithPropertyChanged

@wing328 wing328 merged commit a050907 into swagger-api:master Dec 21, 2017
@wing328 wing328 changed the title fix net35 with JsonSubTypes [C#] fix net35 with JsonSubTypes Dec 21, 2017
@manuc66 manuc66 deleted the feature/fix-net35 branch December 21, 2017 20:02
slarti-b added a commit to slarti-b/swagger-codegen that referenced this pull request Dec 26, 2017
* master:
  move bvwells to go tech comm
  update current stable version in readme
  skip push snapshot to avoid error
  comment out checkstyle in circleci pom.xml
  comment out check style plugin
  Revert "update version to 2.4.0-SNAPSHOT"
  update ci config to install codegen locally
  update doc to 2.3.0
  update version to 2.4.0-SNAPSHOT
  update version to 2.3.0
  fix net35 with JsonSubTypes (swagger-api#7043)
  [Javascript] Set ES5 as default (swagger-api#7239)
  swagger-api#7201: Take the modelPropertyNaming property into account again (swagger-api#7202)
  add trenneman as elm creator
  add bvwells to go technical committee
  Video "Building an API with Swagger" (swagger-api#7237)
  [Akka-Scala] This is a fix to a bug introduced by this PR swagger-api#7172 (swagger-api#7228)
  Add Elm language - BETA (swagger-api#6947)
  bump stack resolver to lts-10.0 (swagger-api#7221)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants