search

AArnott / CodeGeneration.Roslyn

Assists in performing Roslyn-based code generation during a build.
Microsoft Public License
408 stars 59 forks source link

Add ubuntu CI test suite #98

Closed Pzixel closed 5 years ago

Pzixel commented 5 years ago

This is an attempt to build against Ubuntu host to make sure netcore library actually works. See #96

Pzixel commented 5 years ago

I tried to port tests on netcore but I get very subtle errors that I have no netcore on my machine...

AArnott commented 5 years ago

The build failure is because one of the target frameworks is .NET Framework itself, which isn't available on Ubuntu. So the build instructions on Ubuntu have to be carefully written to use dotnet build -f netcoreapp2.0 or whatever framework you want to target that doesn't include the desktop ones. And you can't dotnet pack since that requires all frameworks to be built.

If you can fix it up, I'll accept the PR. If I end up doing it, I'll be switching from AppVeyor to Azure Pipelines at the same time.

Pzixel commented 5 years ago

The build failure is because one of the target frameworks is .NET Framework itself, which isn't available on Ubuntu.

Yep, I get it. But I failed trying migrate test projects from net462. I know about this switch, but it doesn't really help when you have project targets 462 only :)

Pzixel commented 5 years ago

So I'm not committing nor merging this branch now. Being said, it's better for you just commit in it until it's done. I enabled the checkbox

image

So you can easilty commit in the same branch without having it in master (incomplete and broken).

AArnott commented 5 years ago

OK, I usually multitarget my tests, so I assumed I had done so here. I guess I didn't.

Pzixel commented 5 years ago

No. And when I tried to move it to netcore it failed with weird errors from .targets file (about minimum netcore version and so on). I played a bit with it, but didn't succeed. I then thought that it could be your kind of magic with get versioning, mstasks and so on so I didn't investigate it a lot.

AArnott commented 5 years ago

Hmm... well, simply adding netcoreapp2.1 as a target framework to the test project allowed it to build immediately for me. The minimum netcore version you speak of might be that the build agent you're using doesn't have the 2.1.300 SDK installed, which the build requires. That is easily remedied. I'll see what I can do.

Pzixel commented 5 years ago

Well, I had 2.1.402, but it didn't work for some reason.

P.S. Do you mind keeping all these crossplatform targeting? I mean nowadays nobody is using portable or WP7.5 targets. Full framework + netcore is enough, I guess.

Great to see it works. However, in this case I don't see why it doesn't work to me. There are tests for dependencies IIRC so it should. But it doesn't.

AArnott commented 5 years ago

The tests are not passing for me anywhere. AppVeyor hasn't been running them. I think the first order of business is to get them passing.

AArnott commented 5 years ago

Well, I had 2.1.402, but it didn't work for some reason.

That's because my global.json file specifies the SDK version to use, so it must be exactly 2.1.300.

Pzixel commented 5 years ago

That's because my global.json file specifies the SDK version to use, so it must be exactly 2.1.300.

Now that makes sense. However, I don't see why not use carret semver. Not everyone has exactly the same version as you :)

The tests are not passing for me anywhere. AppVeyor hasn't been running them. I think the first order of business is to get them passing.

I was confused by little green checkbox saying appveyor build passed. Now I see )

AArnott commented 5 years ago

However, I don't see why not use carret semver. Not everyone has exactly the same version as you :)

That isn't supported by global.json. Besides, the whole point of pinning it is to prevent it from floating. For all the same reasons that NuGet defaults to always restoring the same package versions across time, and Yarn pushes for checking in their lock files. It's all about build reproducibility. If .NET Core SDK revs and breaks something, I don't want all my builds suddenly on the floor with no idea why. I know that this particular version works, so that's what I always use till we have a chance to test a newer version of the SDK. Ya, it's a bit of a pain at first now that 2.1.300 is no longer the "latest" since folks need to install that particular version of the SDK, but it's not too bad, IMO. It's better than builds periodically failing for no apparent reason.

Pzixel commented 5 years ago

@AArnott it's much more painful than you could expect. I'd rather propose to remove exact version pinning and leave it up to user, but fix the version on which tests are working. So you always know it works for 2.1.300, but user is free to use any other version. If something doesn't work then he could try reproduce it on 2.1.300, but if it does work then everything is golden.

Seriously, users need to track what version is now intended for installation just to make it compile? Bizzare :)

In a nutshell: instead of actively breaking the build with unsupported version just allow then do what they want. Instead you fix this version in CI tests only so you can always know that your build works for some particular version.

This approach is strictly better because:

  1. Users that have the same version don't see any difference
  2. Users that have another version where the code breaks don't see any difference
  3. But Users that have another version where code actually works on different version now are happy

Another bonus is that currently you have "Mandatory install 2.1.300 or compilation would fail" which changes to "If compilation fails, install 2.1.300" that sounds much more reasonable.

Don't you think?

AArnott commented 5 years ago

So you always know it works for 2.1.300

How would I know it works on 2.1.300 if we're not building on it regularly? Note I'm not pinning the runtime version -- I'm pinning the SDK version used to build the thing.

Seriously, users need to track what version is now intended for installation just to make it compile? Bizzare :)

People are moving more in this direction, actually. That's why the .NET SDK comes in packages nowadays. The only real pain point here is that this part of the SDK doesn't automatically download when people try to build with it. That's something I expect will change in the future. Then it will just quietly download just like nuget packages you reference that don't happen to be the versions you already have in your cache.

Your proposal sounds reasonable, but I don't know how to do it. The only way to control for the SDK version AFAIK is the global.json file, which is a source controlled artifact. That means it will apply to local builds as well as CI builds.

I'm going to reach out to the .NET Core SDK folks and ask them why they produce build warnings encouraging folks to check in this file, yet don't have a better story for folks who don't already have the SDK installed.

AArnott commented 5 years ago

In the meantime, you can download the right SDK from here: https://www.microsoft.com/net/download/dotnet-core/2.1

Pzixel commented 5 years ago

Any progress on that? I have a heavy need in ubuntu support since I'm building a docker app where it just doesn't work.

AArnott commented 5 years ago

Any progress on what? Getting the .NET Core folks to auto-install the SDK version? Even if they do that, it will likely be months before it's available.

This shouldn't be blocking you. Just arrange for your docker image used to build your app to install the required SDK. I do that in my linux-buildagent docker image here:

https://github.com/AArnott/linux-buildagent/blob/026791b7e1226db8aa315f97a147187a1b6fd437/Dockerfile#L29

Pzixel commented 5 years ago

@AArnott I'm not sure what SKD are you talking about.

My image is based on microsoft/dotnet:2.0-sdk, see https://github.com/Pzixel/Solidity.DotNet . Here is a build that fails because of problems with dependencies shown above: https://circleci.com/gh/Pzixel/Solidity.Roslyn/58

So I don't know what issue with SDK you mention. It's about dependency-resolution algorithm. I'm trying to make it compile, but without any success. I'd appreciate any help if any.

AArnott commented 5 years ago

Oh, OK. I don't know how I was supposed to know what you were asking about. Our discussion had been focused on the 2.1.300 .NET Core SDK until your last message. Now it seems you're talking about your code generator which itself has an external dependency, which is entirely unrelated to this PR or the discussion we've been having.

I've never seen generator's own dependencies get loaded properly. @LokiMidgard added a feature to make this work in https://github.com/AArnott/CodeGeneration.Roslyn/pull/62 but I haven't personally used it.

Pzixel commented 5 years ago

I linked it from #96 so I thought it provides some context. Sorry for that, I wasn't clear enough.

I've never seen generator's own dependencies get loaded properly. @LokiMidgard added a feature to make this work in #62 but I haven't personally used it.

The interesting point here is that I actually have a generator that uses Json.Net just fine. It happens because generator itself seems to be using it. But if I add yet another project that relies on Json.Net then resolution fails.

It's interesting that instead of always-failing or always-succeeding scenario we have some middle-earth.

I have seen #62 but I have no idea how to use it. There is no examples, docs or something. I looked at PR but it wasn't enough to me to get it.

I propose to move to #96 because this one has been merged and it's kinda irrelevant here.

AArnott commented 5 years ago

Dependencies will work (even if fundamentally broken) if something earlier in the build loaded them into the same appdomain that you're running in. That may explain why it sometimes works for you.

Pzixel commented 5 years ago

Yep, I thought about it, but it doesn't explain why sometimes it doesn't load the same assembly. I thought it may be an issue with versions, but even if I set exactly same version it doesn't load if it's "dependency dependency" instead of direct one.

Pzixel commented 5 years ago

Any progress on making it pass on Ubuntu?

AArnott commented 5 years ago

I've had tests passing on Ubuntu 16 and 18 for a while.