-
Notifications
You must be signed in to change notification settings - Fork 218
Enforce that DLL/EXEs in platform manifest have a FileVersion #5682
Conversation
Ah... CI failing because crossgen still strips the version on non-Windows as it always has. I'm thinking we should actually take #5667 too for a little more source-build correctness. In source-build, we can't hop over to Windows to build the platform manifest. It already isn't accurate for other reasons (https://github.com/dotnet/core-setup/issues/5297), but might as well fix this one. |
Holding off on adding #5667, crossgen shouldn't be stripping version on non-Windows anymore. @davidwrighton is looking at fixing it--I'll wait for that. Once an auto-PR comes through with the change, this PR should go green. |
I misdiagnosed that! It's just
(Edit: moved info to https://github.com/dotnet/corefx/issues/36630.) |
That's interesting. Suppress it for now and let's file an issue on CoreFx. |
Sounds good, filed https://github.com/dotnet/corefx/issues/36630. (Was already working on it. 😄) |
Interesting that it isn't missing the version, but the version is set and it is 0.0.0.0 |
a868fe2
to
7c016cc
Compare
@@ -158,6 +158,14 @@ | |||
<Output TaskParameter="TargetOutputs" ItemName="SharedFrameworkRuntimeFiles" /> | |||
</MSBuild> | |||
|
|||
<!-- | |||
Workaround: zero-versioned Microsoft.VisualBasic.dll in non-Windows CoreFX transport package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually thinking you would constrain to a single filename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be better, but I'm not sure if the risk of another non-Windows file changing to 0.0.0.0/none before https://github.com/dotnet/corefx/issues/36630 is fixed is high enough to warrant effort here. This is also a backstop for a brand new source-build-only fix that we stumbled into (for a bug nobody has mentioned before) rather than actually preventing a regression.
Not hard to do, but this was a bit quicker and I wanted to get this out of the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. One thing I've thought about in the past is constructing the platform manfiest from some form of authoring rather than inspecting binaries. That way we just inspect the binaries in the individual legs to validate that they are represented correctly in the manifest, but the manifest represents a superset of all legs. When maestro picks up an update it runs a target that commits an updated manifest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it might make sense to do more "layout correctness" checks in a central place. Things like this and verify closure are relevant for more things than just the places where we run them now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, all this stuff happening recently has been making me think about how core-setup could really use general artifact testing, rather than trying to add little checks to various individual parts of the build.
When maestro picks up an update it runs a target that commits an updated manifest.
Seems like this would make the build somewhat faster, too, since we wouldn't have to download all platforms' runtime packages on all legs to generate the platform manifest.
Unfortunately not supported in Maestro++ unless it's coded into Darc directly. dotnet/arcade#774. I think it'd have to be pretty general and robust because that limits our opportunities to change/maintain it quite a bit. I think there's also expectations of "fast" and not requiring a real repo clone and build tooling. I can only imagine it being reasonable if everything needed is in BAR metadata.
I wondered a bit about the style of baseline + update baseline target + PR validation build letting you download an updated baseline (we use this in source-build). Problem is the versions change for every single update--so I don't think that would work at all.
…/core-setup#5682) * Enforce DLL/EXEs in platform manifest have version * Allow zero/missing versions on non-Windows Commit migrated from dotnet/core-setup@851859e
Prevent https://github.com/dotnet/core-setup/issues/5665 from occurring again by making sure no DLL/EXE FileVersions are
0.0.0.0
.Not enabled for WindowsDesktop. There are some files to investigate there. We might need to have a baseline to enable the check there if there are valid reasons for the files not to have versions.
I rebased it back before the crossgen issue was fixed and it was able to detect the issues (full list):