Skip to content

Fix System.IO.Ports native package not getting published any longer after the oob + sfx split#66176

Merged
joperezr merged 1 commit intodotnet:mainfrom
joperezr:FixIOPortsPackage
Mar 4, 2022
Merged

Fix System.IO.Ports native package not getting published any longer after the oob + sfx split#66176
joperezr merged 1 commit intodotnet:mainfrom
joperezr:FixIOPortsPackage

Conversation

@joperezr
Copy link
Member

@joperezr joperezr commented Mar 4, 2022

FYI: @ViktorHofer @ericstj @mmitche

The native component package of System.IO.Ports doesn't get build in the all configurations leg, and instead should be build in individual Unix verticals. With the recent changes to split sfx and oob builds, these packages builds got moved into the all configurations leg in Windows which would never build the unix packages. The changes in this PR will fix this and have the native packages build on the individual verticals as they were before.

@ghost
Copy link

ghost commented Mar 4, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

FYI: @ViktorHofer @ericstj @mmitche

The native component package of System.IO.Ports doesn't get build in the all configurations leg, and instead should be build in individual Unix verticals. With the recent changes to split sfx and oob builds, these packages builds got moved into the all configurations leg in Windows which would never build the unix packages. The changes in this PR will fix this and have the native packages build on the individual verticals as they were before.

Author: joperezr
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@joperezr
Copy link
Member Author

joperezr commented Mar 4, 2022

Thanks @carlossanlop for the help investigating.

@carlossanlop
Copy link
Contributor

@dreddy-work what do we need to do to unblock you after this gets merged to main?

@joperezr
Copy link
Member Author

joperezr commented Mar 4, 2022

We don’t need to do anything to unblock them, dependency flow will update that PR with the versions that have the fix

@joperezr joperezr added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 4, 2022
@joperezr
Copy link
Member Author

joperezr commented Mar 4, 2022

Adding the No-Merge label until I validate on the CI builds that the changes actually produce the packages as expected. Once I validate this I’ll remove the label and merge.

@joperezr joperezr removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 4, 2022
@joperezr joperezr merged commit 3b72c25 into dotnet:main Mar 4, 2022
@joperezr joperezr deleted the FixIOPortsPackage branch March 4, 2022 03:56
@ViktorHofer
Copy link
Member

Thank you, I missed that during my refactoring. It's a shame that we don't have PR validation to make sure the expected set of packages are generated. Ie a restore of the System.IO.Ports would have raised this issue. Unsure if it's possible but maybe we could have package feeds just for PR validation? cc @mmitche

@joperezr
Copy link
Member Author

joperezr commented Mar 4, 2022

That is exactly what we do when we run our package validation tests, the only package we don’t run this validation on is System.IO.Ports, and the reason is that we don’t have one build that we can run that test on which has all the required packages for restore to work:

<ExcludePackages Include="runtime.native.System.IO.Ports" />

since this is special cased there already, perhaps we can also special case a test on individual verticals that are expected to produce these which would validate that they actually got generated.

@ViktorHofer
Copy link
Member

Right. What I was trying to say is that it might be possible to make this work and remove the validation exclusion by sequencing when testPackages runs differently. There are obvious downsides to that but it should at least be discussed :)

@ghost ghost locked as resolved and limited conversation to collaborators Apr 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants