Runtime: Package publishing is dodgy

[ericstj]

During official builds it looks like all legs overwrite files when copying to azure storage. This creates some unreliable "last in wins" behavior where the last build to succeed will be the one that gets published.

In 1.0 and 1.1 folks tried to workaround this by turning off some of the overlapping package builds on non-windows, but this is contrary to the vertical build goals. Those goals drive us to have a reference package produced by all verticals, but we'd really only want to ship the one from Windows since it will be signed.

I think we need to look at a way to make this work without adding extra complexity to developer builds, or making the official build different from developer builds. Here's my crack at a suggestion for how this should work.

1. Builds should copy to a leg-specific path in azure. This eliminates the potential for overwriting.
2. During publishing have a precedence for package sources, as follows.
   a. first prefer windows vertical build legs.
   b. then prefer windows all non vertical build legs.
   c. then consider other OS legs.
3. If there is ever an ambiguous choice that should be an error: two legs produce the same package without a clear precedence based winner. Fail the build.

/cc @chcosta @dagood @weshaggard

[weshaggard]

This is actually causing a real issue right now which is manifesting itself in assemblies with incorrect identities. see https://github.com/dotnet/corefx/issues/16152.

[dagood]

/cc @dleeapho I think this is something we should support in BuildTools. Some background: right now, our publishing code is in the VSTS definition, and it uses tool acquisition scripts in the build pipeline repo.

[dleeapho]

I agree it would be good to put package publish into BuildTools, that way CoreClr, Core-Setup, WCF(?), and CoreRT could leverage the more robust package publish.
@weshaggard , @ericstj since this seems to be a hot item, should this be fixed in CoreFx first then move it to BuildTools?

[ericstj]

That's up to you, but I don't think it should live in corefx outside whatever hooks you need to invoke it.

[ericstj]

We might need a quick fix to this as our we're hitting it now consistently in daily builds. As a workaround we might be able to condition this package to not be produced on official builds that don't sign ☹️

[weshaggard]

I think this is something we should support in BuildTools. Some background: right now, our publishing code is in the VSTS definition, and it uses tool acquisition scripts in the build pipeline repo.

@dagood how do you envision this getting fixed? It seems like no matter what we do if multiple legs of build definitions are producing the same packages then someones to tell the publish step which one to publish. Do we envision that being done as a validation step that fails if we produce for publishing the same packages on multiple legs or some other configuration mechanism to configure what gets published?

[dagood]

I like @ericstj's proposal, where the precedence order is applied and conflicts caught during the package publish build definition. A way to do this without changing too much about the publish def is to use a buildtools target/task to find the "to publish" packages in the sync -ab'd dir and copy them to an intermediate directory. Then, the steps we have now would operate on that.

The workaround of disabling packages being produced that we don't want to publish also sounds reasonable, and I would think we could turn off OverwriteOnPublish in the corefx defs to make sure the legs create the right set.

[weshaggard]

While the order preference could work it feels a little too fragile (aka magical) to me. I would much prefer we just fail on any package conflicts and we encode in the build of the packages somehow that this is an official package build and we can then encode the logic of which one to chose in our package traversal build and disable (or bin-place outside the publish folder) the ones that we don't want to publish.

[dagood]

I don't have a preference one way or the other myself, but that sounds like the approach @ericstj called a workaround and wanted to avoid. (I also don't think buildtools would be involved with that plan.)

[weshaggard]

I would prefer we have less magic going on in our tools here and we be more explicit. So I suspect all we need from the tools prospective is some property we can determine official builds on and some logic that fails the build if we find conflicts.

[ericstj]

I'm ok with what Wes says so long as we have a good agreed upon condition to use for "Official" builds. Can we agree on that here? OfficialBuild == true?

I also second what Wes says about conflicts. It must be a failure if one leg tries to produce the same package as another leg in this case.

[ericstj]

Ping

[dagood]

This part of the work is something I could do:

some property we can determine official builds on and some logic that fails the build if we find conflicts.

Adding a property to the checked-in build defs or pipeline.json and changing these instances of OverwriteOnPublish to false: https://github.com/dotnet/corefx/search?utf8=%E2%9C%93&q=OverwriteOnPublish.

That would fail without this part of the solution, though, which I can't do quickly:

encode in the build of the packages somehow that this is an official package build and we can then encode the logic of which one to chose in our package traversal build and disable (or bin-place outside the publish folder) the ones that we don't want to publish.

@ericstj If you can do the second part (OfficialBuild sounds fine to me), I could add the pipeline changes.

[ericstj]

Sure, please add pipeline changes. LMK when they are in then I will use that property.

[dagood]

Here are the pipeline changes: https://github.com/dotnet/corefx/compare/master...dagood:fix-package-overlap. I tried a build of the OSX definition and it looks like it gets passed correctly.

@ericstj I think it's best if you make your changes on top of my commit and submit it as a single PR. Otherwise, I'd have to split off the OfficialBuild=true for a first PR, then you'd do your PR, and I'd disable overwriting in a third PR. Thanks.

[weshaggard]

Do we think we need a unique property for this or just OfficialBuildId will be enough?

[dagood]

My concern would be a scenario where we want to specify the version with OfficialBuildId but build all packages on a platform, like maybe build from source. If we do that in another way, though, and define Official strictly as the CoreFX-only official build, that sounds fine to me.

[weshaggard]

Yes I guess that is a possibility but I would much prefer we use something other then OfficialBuildId for our build from source work as using it there would change the meaning of what it implies. If we need something to control the version number for build from source I would prefer we define another version property for that.

[dagood]

Sounds good. Created https://github.com/dotnet/corefx/compare/master...dagood:fix-package-overlap-2 to pass OfficialBuildId instead.

[MattGal]

This issue may also be causing official build failures randomly due to attempting to write the same blob at once. We should attempt to get blob naming unique as soon as possible so as to prevent this.

[ericstj]

Sorry, this dropped off my todo-list. PR opened that conditions the packages:
https://github.com/dotnet/corefx/pull/18081

[weshaggard]

What is left for this work item? @MattGal is the only thing left to do is disable overwriting of files to blob storage? If so can you please submit a PR to do that?

[MattGal]

I believe that's already done via implementing suggestion 3, by https://github.com/dotnet/corefx/commit/590e7fbd9b93b12891674b99327ad114c5800b8c for CoreFX. When OverwriteOnPublish is false, we will throw an exception for any blob we try to upload that already exists, so dotnet/corefx#3 on Eric's list is done.

If we want to do something for 1, "Builds should copy to a leg-specific path in azure" we could enable this but I think it's unnecessary now.

[weshaggard]

I missed the OverwriteOnPublish parameter in @dagood's PR (it was off the screen). Given that I'm going to call this issue complete then, because I assume the build would have already been failing if we are still publishing packages from multiple legs.