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

Introduce pre-caching into installer #10037

Merged
merged 18 commits into from
May 10, 2021

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Mar 26, 2021

I can almost guarantee this doesn't work, but opening anyway so you can take a look.

@Forgind
Copy link
Member Author

Forgind commented Apr 1, 2021

As I mentioned to @wli3, this likely is not the best project. I'm thinking of changing it to a solution that references all the dotnet new ___ projects, builds them, and creates a set of caches—one for each—that the user can read in and combine. I could also combine them into one cache, though that can be cleaner or more efficient depending on how I do it—the easiest method would be to create a cache per project, then call RAR again, reading in all the caches and outputting them to a single cache that we actually keep. It's a little inefficient, though.

Also, I expect this to fail until dotnet/msbuild#6311 has been merged.

<SampleProjectForResolveAssemblyReferenceCache Include="$(RepoRoot)\TestAssets\TestProjects\RARPreCacheProject\**\*.*" />
</ItemGroup>
<Copy SourceFiles="@(SampleProjectForResolveAssemblyReferenceCache)" DestinationFiles="$(RepoRoot)\artifacts\bin\redist\$(Configuration)\RARPreCacheProject\%(RecursiveDir)%(FileName)%(Extension)" />
<Exec Command="$(RepoRoot)\artifacts\bin\redist\$(Configuration)\dotnet\dotnet.exe msbuild $(RepoRoot)\artifacts\bin\redist\$(Configuration)\RARPreCacheProject\RARPreCacheProject.csproj /t:ResolveAssemblyReferences /p:AssemblyInformationCacheOutputPath=$(SdkInternalLayoutPath)sdk\$(Version)\SDKPrecomputedAssemblyReferences.cache" />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a better existing property for $(RepoRoot)\artifacts\bin\redist$(Configuration)\dotnet\dotnet.exe (stage2 SDK location)

And you should only run this on a windows leg

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not you mention it, I think it's equivalent to $(SdkInternalLayoutPath)dotnet.exe. I can change it to that.

Why only on a windows leg? I think it should work for any OS.

@Forgind
Copy link
Member Author

Forgind commented Apr 5, 2021

Also: the change that (I think) will make this work is here.

@Forgind Forgind marked this pull request as ready for review April 29, 2021 05:29
<ItemGroup>
<AssembliesToResolve Include="$(RedistLayoutPath)\**\*.dll" Exclude="$(RedistLayoutPath)\**\native\**\*.dll" />
</ItemGroup>
<ResolveAssemblyReference AssemblyFiles="@(AssembliesToResolve)" Silent="false" AssemblyInformationCacheOutputPath="$(RedistLayoutPath)sdk\$(Version)\SDKPrecomputedAssemblyReferences.cache" SearchPaths="{RawFileName}" />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no running stage 2 anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stage 2 MSBuild engine isn't expected to work on ARM, so we can't use the full stage 2 MSBuild as I originally had it. Using the stage 2 tasks assembly (which is what matters) with the stage 0 engine could work, but it isn't great to be intentionally using different versions of the same product in a single call.

This goes a different route and just updates the stage 0 sdk to the minimum we would need to make the cache. Reading the cache is harder—I don't believe that will work with either the stage 0 or stage 2 sdk at this point. It needs dotnet/msbuild#6395 at minimum and dotnet/msbuild#6393 if you installed to Program Files or another folder that requires special write privileges. Fortunately, reading the cache should only happen with the stage 2 sdk, so we won't need a second PR to update the stage 0 sdk again afterwards.

The last viable route we could have gone is to use the stage 0 SDK to build a project that finds the stage 2 tasks assembly and uses that to call RAR. That's a viable option, but it's more complicated (read: I tried for a few hours and didn't get it to work) and slower, and all it would achieve is not needing to update the stage 0 SDK. I don't think updating the stage 0 SDK is bad, so I prefer the current PR.

Let me know if any part of that was confusing 🙂 Long answer to a short question.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Looks good

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@wli3
Copy link

wli3 commented May 10, 2021

I approve since this change looks harmless and I don't want to block you due to my OOF. But that is all? I expect a call to stage 2 at least

@wli3 wli3 merged commit e8fa33c into dotnet:main May 10, 2021
@Forgind Forgind deleted the create-precomputed-rar-cache branch May 10, 2021 17:35
Forgind added a commit to dotnet/msbuild that referenced this pull request May 24, 2021
Context
The precomputed cache from dotnet/installer#10037 lives in Program Files after it's installed on a new computer. Program Files can only be accessed with admin privileges, which not all users have and those that have generally wouldn't expect. This permits reading the precomputed cache even without admin rights.

Changes Made
new FileStream(stateFile, FileMode.Open) opens the file as if you had read/write access but only actually grants you read permissions. It still requires administrator privileges, however, if a file requires administrator privileges to write to. This removes that requirement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants