Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate 6.0 SDK missing file: shared/Microsoft.NETCore.App/6.0.0/.version #2569

Closed
dagood opened this issue Oct 25, 2021 · 9 comments
Closed
Assignees
Labels
area-product-experience Improvements in the end-user's product experience

Comments

@dagood
Copy link
Member

dagood commented Oct 25, 2021

In the Microsoft-built SDK, this file contains the commit hash on the first line and then the version number on a second line. I don't think anyone uses it (it's just informational) but it would be good to fix this diff if it's a small thing.

dagood#9 (comment)

@eerhardt
Copy link
Member

eerhardt commented Oct 26, 2021

It is used here:

https://github.com/dotnet/sdk/blob/00ddecd515898f3c6e0a6d325d018d98a610851f/src/Cli/Microsoft.DotNet.Cli.Utils/DotnetFiles.cs#L21

I actually have it in my local build:

[root@7114784af2d1 workdir]# ls -al /src/DotNetTest/dotnet/sdk/6.0.100/
total 35464
drwxr-xr-x 27 root root   16384 Oct 26 01:35 .
drwxr-xr-x  3 root root    4096 Oct 26 01:33 ..
-rw-r--r--  1 root root      71 Oct 26 01:32 .toolsetversion
-rw-r--r--  1 root root      63 Oct 26 01:33 .version
drwxr-xr-x  2 root root    4096 Oct 26 01:35 AppHostTemplate

Maybe it is hidden because it starts with a .?

UPDATE: Oops - I was looking in the sdk folder, but this is the shared fx .version file. Disregard the above.

@eerhardt
Copy link
Member

I can't find anyone who uses the shared fx .version file. The SDK gets the version by reading the folder name:

https://github.com/dotnet/sdk/blob/a30e465a2e2ea4e2550f319a2dc088daaafe5649/src/Cli/Microsoft.DotNet.Cli.Utils/Muxer.cs#L16-L22

That said, I agree we should have the same files as the Microsoft build.

@dagood
Copy link
Member Author

dagood commented Oct 26, 2021

The shared/Microsoft.NETCore.App/6.0.0/.version file is missing because runtime source-builds with DisableSourceLink=true:

https://github.com/dotnet/runtime/blob/4822e3c3aa77eb82b2fb33c9321f923cf11ddde6/eng/SourceBuild.props#L26-L32

  <ItemGroup>
    <!-- Work around issue where local clone may cause failure using non-origin remote fallback: https://github.com/dotnet/sourcelink/issues/629 -->
    <InnerBuildEnv Include="EnableSourceControlManagerQueries=false" />
    <InnerBuildEnv Include="EnableSourceLink=false" />
    <InnerBuildEnv Include="DisableSourceLink=true" />
    <InnerBuildEnv Include="DeterministicSourcePaths=false" />
  </ItemGroup>

https://github.com/dotnet/arcade/blob/70de8c7a1a5230d4dc8e273dcb0057805165ae19/src/Microsoft.DotNet.SharedFramework.Sdk/targets/sharedfx.targets#L425-L427

Re-enabling sourcelink is tracked in source-build:

In theory, the underlying bug (dotnet/sourcelink#629) can be treated as just a missing dev scenario--you are unable to use custom remote names while running ArPow. It could be ignored and sourcelink turned back on in dotnet/runtime. I've submitted dotnet/installer#12527 to give it a try.

@dotnet/source-build-internal @omajid, thoughts on the dev scenario, if it's worthwhile to take this as a 6.0.0 patch if it doesn't regress anything?

@dagood
Copy link
Member Author

dagood commented Oct 26, 2021

Note, the Fedora 34 5.0 SDK does contain this .version file. The diff seems to be new to ArPow.

@eerhardt
Copy link
Member

@jkoritzinsky - is there a reason that the _CreateVersionsFile target in sharedfx.targets is conditioned on Condition="'$(DisableSourceLink)' != 'true'"?

  <Target Name="_CreateVersionsFile"
          DependsOnTargets="InitializeSourceControlInformationFromSourceControlManager"
          Condition="'$(DisableSourceLink)' != 'true'">

@jkoritzinsky
Copy link
Member

That was ported over from the initial infrastructure that @dagood implemented in 3.1. I believe it's disabled because the InitializeSourceControlInformationFromSourceControlManager didn't exist in the old source-build model when building from source.

@dagood
Copy link
Member Author

dagood commented Oct 26, 2021

I think it'd be reasonable to change the condition/targets to remove InitializeSourceControlInformationFromSourceControlManager rather than removing the _CreateVersionsFile target entirely when '$(DisableSourceLink)' != 'true'. Then _CreateVersionsFile could use the source-build-provided Git data to create the .version file (and only skip creating the file if these properties aren't defined): https://github.com/dotnet/installer/blob/c020befd81ed503c4ef5784e59872b3bfa45f881/src/SourceBuild/tarball/content/repos/Directory.Build.props#L91

But ideally sourcelink could just be enabled, to avoid special-casing it. Normally, it seems pretty reasonable to assume that if sourcelink isn't available, the commit hash (if any) isn't discoverable.

@dagood dagood self-assigned this Oct 27, 2021
@dagood
Copy link
Member Author

dagood commented Oct 27, 2021

We're planning on leaving this diff in the 6.0.100 source-built SDK and not adding a patch yet. (dotnet/installer#12527.) This will help 6.0.100 stability. We've done smoke-testing and manual validation on what we have now, and haven't found any issues with leaving this file out.

Seems like we can add a fix in 6.0.101/next servicing release.

@MichaelSimons
Copy link
Member

Closing as a patch was added in source-build and was backported to runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-product-experience Improvements in the end-user's product experience
Projects
None yet
Development

No branches or pull requests

4 participants