Conversation
|
test UWP CoreCLR x64 Debug Build |
There was a problem hiding this comment.
You shouldn’t be generating the runtime.json yourself. If you have a project reference to a runtime-specific pkgproj we will generate in the pkgproj tooling.
I don’t think the runtime package will build correctly as it is. We need something similar to how we build the framework packages.
We shouldn’t even be using runtime.json here: it doesn’t work at all with shared framework applications. You should use normal package dependencies, or merge the native binaries from multiple build legs into a single package
@ericstj how do I do that? And how do I test it locally? |
|
To do the join you'd actually have to refactor the build of CoreFx. It's a substantial amount of work: you'd need to make sure that the runtime-specific legs publish their binaries to some location, then we run a new leg before packaging that downloads the runtime-specific binaries so that they can be packaged together. Changes would need to be made to the build YAML and you'd need to test by pushing to a branch and kicking off official builds on that branch. @safern could probably provide better details on this than I can. That said, I don't think you should go that far to start. I think its simpler to just remove the runtime.json and make these direct dependencies. That works so long as all packages provide non-overlapping assets (EG: you don't have a linux-x64 and ubuntu-x64 build). I think the easiest way to do this make use of the targets already defined in the pkg folder. To test you'll want to make sure that a vertical build produces a runtime package for the RID you're building for and the allconfigurations build produces a package with references to all the RIDs that will be built by the official build. I think the easiest way to do this using the existing infra is to create a library-> package mapping for your dll name(s) here: That allows are dependency harvesting system to automatically insert a reference to the native package wherever we see it used (rather than you having to try and express in the PkgProj what you're already expressing in your implementation assemblies build configurations). Then create a package heirarchy like the following:
|
krwq
force-pushed
the
system.io.ports_pkg
branch
from
93179eb to
db6cbf2
Compare
|
@ericstj when I did On linux regular build also produces both nupkgs but the rid specific package doesn't contain the .so file as specified in the package index and the pkgproj. Any clues? I'll be digging a bit more tomorrow |
ericstj
left a comment
ericstj
left a comment
There was a problem hiding this comment.
I think I misspoke about allconfigurations build. It looks like the filtering only happens on official builds:
corefx/pkg/dir.traversal.targets
Lines 6 to 12 in a10890f
I made some comments that should address most other issues. I don’t see what here would produce an empty rod specific package (other than Windows condition) can you clarify? Did you examine the System.IO.Ports package to see if we added the dependencies?
| <PropertyGroup> | ||
| <SkipPackageFileCheck>true</SkipPackageFileCheck> | ||
| <SkipValidatePackage>true</SkipValidatePackage> | ||
| <SystemIOPortsNativePath>$(ArtifactsBinDir)\native\$(TargetGroup)-$(DefaultOSGroup)-$(Configuration)-$(ArchGroup)</SystemIOPortsNativePath> |
| <SkipValidatePackage>true</SkipValidatePackage> | ||
| <SystemIOPortsNativePath>$(ArtifactsBinDir)\native\$(TargetGroup)-$(DefaultOSGroup)-$(Configuration)-$(ArchGroup)</SystemIOPortsNativePath> | ||
| </PropertyGroup> | ||
| <ItemGroup Condition="'$(OS)' != 'Windows_NT' and '$(PackageTargetRuntime)' != ''"> |
There was a problem hiding this comment.
Windows condition here is bad. You are getting a Windows build because of this behavior:
corefx/pkg/Directory.Build.props
Lines 16 to 24 in 3685052
The purpose is to let folks build a package on a platform that may not be officially suppoted yet. That may not be something you want. You can have a property that turns this itemgroup off and instead just sets buildrid to officialbuildrid.
There was a problem hiding this comment.
I think I'll go with explicit PackageTargetRuntime check
There was a problem hiding this comment.
Another thought: change the item name in your Rid.props file to use BuildRID instead of OfficialBuildRID and it will avoid this behavior.
pkg/runtime.native.System.IO.Ports/runtime.native.System.IO.Ports.pkgproj
Show resolved
Hide resolved
|
Thanks a lot @ericstj, I'll update and test first thing in the morning! |
|
I think this looks correct now:
|
|
test corefx-ci (Linux x64_Release) |
|
@dotnet/dnceng how do you restart "corefx-ci (Linux x64_Release)"? I've tried dotnet-bot invocation and "Rerun this check" in the Checks page and both don't seem to work The issue in the check: |
krwq
force-pushed
the
system.io.ports_pkg
branch
from
935593e to
76c094a
Compare
|
For azure pipelines builds the comment syntax is: /AzurePipelines run (i.e /AzurePipelines run corefx-ci) Short form can be "/azp run corefx-ci". You can also do /azp help or in the long form /AzurePipelines help |
|
However note that comment triggers are broken now for forked PRs. It is supposed to be fixed and the fixed was rolled out on Monday. We should be getting it by Monday the latest according to what they told me. I have this issue to track it: https://github.com/dotnet/corefx/issues/35121 |
|
Btw the linux test failures in Fedora is a known issue: |
|
Looks like you may need to suppress package testing since the rid specific packages won’t be available during all configurations build. Hmmmm. Nuget doesn’t have a good way to suppress a dependency: we could publish an empty lower version of the runtime package and force downgrade it. @safern this is another side effect of not having a join in corefx build legs. |
|
@ericstj does it also check versions? If not then I can also comment out all dependencies and remove package index changes from this PR, push it to produce packages and then revert this change. |
|
@ericstj what do you mean by |
|
build a 0.0.0 version of Once we get one official build out of corefx we can replace the 0.0.0 with that (still a downgrade but testing the real scenario more closely) |
|
@ericstj, I have disabled the testing for now, once we get the real package I'll figure how to do the downgrade correctly |
|
If you want to rerun an individual job, it has to be manually through the UI. Through comments you can only do the whole pipeline. I’ve reported that issue to azure devops team. |
|
@safern I already have but even though it shows running on the page it still is red in here |
|
Yes they have a bug in that as well which I have reported, to give you context: a job can have Name and DisplayName. Name has to be unique across all the jobs in the same pipeline, but DisplayName is what is used in the UI to show the job as. (Like a better name). However when the job finishes it will update the failing job. They’re investigating and submitting a fix. |
|
@safern interestingly after CI finished now it's green, so only "in-progress" doesn't work |
|
Yeah. The problem is as I described above, running uses Job.Name, finished uses Job.DisplayName, that is the difference. |
| <BuildRID Include="linux-arm64"> | ||
| <Platform>arm64</Platform> | ||
| </BuildRID> | ||
| <BuildRID Include="linux-x64" /> |
There was a problem hiding this comment.
Don't you need to set the platform for linux x64 and osx as well?
There was a problem hiding this comment.
I copied this from other file and removed stuff I didn't need so I presume no (also I think the output looked correct too). I think for osx and linux we don't have 32bit except ARM
| <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), Directory.Build.props))\Directory.Build.props" /> | ||
| <PropertyGroup> | ||
| <SkipPackageFileCheck>true</SkipPackageFileCheck> |
There was a problem hiding this comment.
Could we add comments above each of this skips to say why is it that we need them?
There was a problem hiding this comment.
I'm actually not sure, I copied the whole project from elsewhere and left it there. I'll try to remove this in a subsequent PR and if not possible I'll add a comment. Let's leave it as is for now since I don't want to block on the comment for waiting on the official build
There was a problem hiding this comment.
If this is actually not required I would rather remove it now, that way we will be able to catch a possible validation error now that we wouldn't have if we kept these guys. Basically what you are doing here is kind of a skipping all of the testing validation for this package, so I would rather make sure that validation succeeds in order to get us more confidence. If we indeed need these validation skipped, it would be good to understand why, since it might be due to a package authoring problem.
| <ItemGroup> | ||
| <TestPackages Condition="'$(TestPackages)' != ''" Include="$(TestPackages)" /> | ||
| <ExcludePackages Include="CoreFx.Private.TestUtilities" /> | ||
| <ExcludePackages Include="CoreFx.Private.TestUtilities;System.IO.Ports;runtime.native.System.IO.Ports" /> |
There was a problem hiding this comment.
Won't you still want package testing to happen on System.IO.Ports and just remove it from the runtime specific package?
There was a problem hiding this comment.
System.IO.Ports depends on the runtime.native.* and that depends on the rid specific packages which don't exist yet. I'll try to fix it all in the next PR, let me test if the package is correct first and once the official package is up we can remove some of the hacks from here
|
I tried restoring this package on my machine with the ones produced by CI and got the following error: Seems like we should be getting that wrong dependency version from the system.io.ports.dll assembly version. We should fix that before merging as well. |
|
@joperezr binlog tells me that target GenerateNuSpec coming from Microsoft.DotNet.Build.Tasks.Packaging contains correct dependency to runtime.native.System.IO.Ports (same for all configurations):
Looks like the bug is in that target I suspect this: https://github.com/dotnet/arcade/blame/master/src/Microsoft.DotNet.Build.Tasks.Packaging/src/GenerateNuSpec.cs#L311 somehow ends up changing min version used. @joperezr any clues? |
Here is the full thing I'm seeing being passed to GenerateNuSpec - this looks good IMO |
|
Ok, what you are missing seems to be an entry in the packageIndex.json. Try adding this and build again, it should make it work: That should force the package version on the dependencies to be correct. |
* System.IO.Ports native package * add inputs and outputs * fix build * change the way native package is build * fix pkgproj and props per feedback * NativeFile -> File * add workaround for the package test * exclude runtime.native.Syystem.IO.Ports from package testing * disable also System.IO.Ports * Add BaselineVersion and InboxOn in packageIndex.json Commit migrated from dotnet/corefx@c776f65
5 participants
Contributes to #33374
(technically fixes but there is some follow up)
Remaining work: