Reorganize dependencies and infrastructure for ACS #2278
Conversation
pgrawehr
force-pushed
the
CompilerAsTool
branch
4 times, most recently
from
d7845cd to
2e6a662
Compare
|
@joperezr Added extra notices as discussed. |
| <Authors>The .NET Foundation community</Authors> | ||
| <NeutralLanguage>en</NeutralLanguage> | ||
| <IsPackable>True</IsPackable> | ||
| <PackAsTool>True</PackAsTool> |
There was a problem hiding this comment.
We should also make sure that this tool stays in preview and doesn't go stable (since it is experimental). You can accomplish that by adding the property: <SuppressFinalPackageVersion>true</SuppressFinalPackageVersion> in the .csproj. This way, even when the rest of the repo builds stable packages, this tool will remain as prerelease.
There was a problem hiding this comment.
I did that, but it doesn't seem to do anything. Can you verify with a release build?
However, this now includes a lot of code that's very likely not really used. Need to find a way around this.
This feature isn't used anywhere
So that compiling them works
For some reason, the linux build fails now due to these apparently missing.
Better keep the low version as long as this is experimental
|
/azp run dotnet.iot |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Ok, so I now understand why the build is still producing stable packages even when you applied the property, looks like all projects under tools (and turns out also the ones under devices) aren't using Arcade at all, and I'm working on some changes on top of your PR to fix that but that is unblocking a can of worms unfortunately. I'll be able to push some changes soon, but we may want to reconsider holding the release for this as this would introduce risk as I'm essentially changing how all of these things are built. |
|
cc @pgrawehr ^^ |
| @@ -9,20 +9,44 @@ | |||
| <AssemblyName>acs</AssemblyName> | |||
There was a problem hiding this comment.
I just noticed this project is targeting net6.0, why is that? Should it be net8.0 instead? net6.0 is out of support so we'd want to update that now.
There was a problem hiding this comment.
The entire library still targets 6.0, so this is consistent. But more importantly is the fact that the runtime used affects the compilation. Because acs relies heavily on the internal runtime implementations (eg the native methods called) updating this requires considerable effort and testing.
So this is something for a future update (I'm looking into a slightly different approach there, though)
| <ToolCommandName>dotnet-acs</ToolCommandName> | ||
| <PackageOutputPath>..\..\..\artifacts\packages\$(Configuration)\Shipping</PackageOutputPath> | ||
| <Version>1.0.2</Version> | ||
| <!-- Stay in beta mode --> |
There was a problem hiding this comment.
I used the version you were using, but is there a reason why we are shipping 1.0.2 if we have never shipped a stable 1.0.0? Should we move this back to 1.0.0?
There was a problem hiding this comment.
We never shipped older releases, but one could build them manually. But I don't have a strong feeling on this.
|
Ok, so did a whole bunch of changes in 36afdf7 to fix tools and devices directories not using Arcade to build. From that, I started getting a bunch of style and analyzer errors, so that's all of the changes in the *.cs files, mainly addressing those. @pgrawehr could you pull my changes and build locally and validate everything looks good to you? If you want to do a local "Official stable build" you can run |
|
@joperezr Not good. The build now fails with That package shouldn't be empty... |
|
Oh interesting, not sure why I didn't see that locally. That is because it is trying to create a symbol package (as that is the default on the repo) and this is a tool package. My latest commit should fix that. |
|
I do still think we should retarget the tool and have it target 8.0 |
|
Not sure why the GH action is failing though, I think for that one it's best if you take a peek. |
That happened because the location of the |
Have you read my comment above? I quickly tried, and it doesn't even build, because the entire project still builds using .NET 7.0 (see global.json). I really wouldn't do such an update as part of this PR. |
|
Oh missed that. Yeah I agree, let's get this merged then and I can put up a follow up PR that moves us to 8.0 infra |
|
@joperezr Can you do the merge? I cannot, because the helix build keep failing. We really need to do something about this unstable ADC test. |
|
We should just disable that test as it is not good to setup a pattern to merge on red. |
Make sure the release build only uses the official package, so that it can be used in client applications that don't build Iot.Device.Common from scratch. Due to the way the compiler works, the client application must be built against the exactly same library the compiler itself is built.
Also updates some test settings
Microsoft Reviewers: Open in CodeFlow