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

Arcade does not support /p:OptionalToolSource options #1176

Closed
michellemcdaniel opened this issue Oct 26, 2018 · 22 comments
Closed

Arcade does not support /p:OptionalToolSource options #1176

michellemcdaniel opened this issue Oct 26, 2018 · 22 comments

Comments

@michellemcdaniel
Copy link
Contributor

In the corefx build, we pulled down the OptionalToolSource packages using sync.cmd /p:OptionalToolSource, /p:OptionalToolSourceUser and /p:OptionalToolSourcePassword. With the port to Arcade, these options no longer pull down the optional tools, which are required for ProfileGuidedOptimization in corefx (specifically, applying IBC data to the various assemblies).

@maririos
Copy link
Member

FYI @weshaggard

@dagood
Copy link
Member

dagood commented Oct 26, 2018

Arcade support is tracked in #100. It sounds like this issue does more generally track getting the flow working in CoreFX though... I would have thought that the BuildTools code that AFAIK is still in CoreFX would be used to keep support for this.

@weshaggard
Copy link
Member

It looks like the BuildTools optional tooling stuff hooked onto the Sync target (https://github.com/dotnet/buildtools/blob/0fa455c550abb4f49f4830a7f2fa78609f373fd0/src/Microsoft.DotNet.Build.Tasks/PackageFiles/OptionalTooling.targets#L12) I deleted that target from corefx recently (dotnet/corefx@66392f5#diff-3ad318a39efd856540e8cf813f20ec09) and renamed it to Restore to align with arcade targets. I'll submit a change in corefx to hook up that target again.

@weshaggard
Copy link
Member

dotnet/corefx#33076 should hook up the optional tooling targets again.

@tmat
Copy link
Member

tmat commented Oct 30, 2018

@weshaggard
Copy link
Member

@tmat how to you handle the credentials for the feed? Is that hidden away in the feed guid you are using?

@tmat
Copy link
Member

tmat commented Oct 30, 2018

Yes, the DevOps build task runs with credentials that can access that guid feed.

@dagood
Copy link
Member

dagood commented Oct 30, 2018

If that ends up being the Arcade way of doing it (and it seems fine to me), the local dev scenario should be doc'd as well. (VSTS/AzDO NuGet auth is notoriously hard to get right... but I haven't tried it in a while, so maybe it's better now.)

@tmat
Copy link
Member

tmat commented Oct 30, 2018

What is the local dev scenario for merging optimization data?

@tmat
Copy link
Member

tmat commented Oct 30, 2018

Do you mean just replicating potential failure locally? If so then yes, it's not hard to get the package from the VS feed.

@michellemcdaniel
Copy link
Contributor Author

There really isn't much of a local dev scenario for merging optimization data, other than for testing new functionality before checking in (like how @DrewScoggins is working on getting IBC data processed for Linux jobs). The only place that optimization data is consumed for CoreFx is in the official build.

@dagood
Copy link
Member

dagood commented Oct 30, 2018

Optional tools is also how TestILC is obtained, which people have to repro more often for UWP testing... I think.

it's not hard

It might not be for someone familiar with the infra, but stuff as simple as PATs expiring can be alien and hard to figure out how to solve for devs who are trying to get product work done. In any case, this should just affect the length of the doc. 😉

@markwilkie
Copy link
Member

Perhaps yet another opportunity for a AzDO tasks?

cc/ @chcosta

@tmat
Copy link
Member

tmat commented Oct 30, 2018

I don't think we need a special task for this. The NuGet one already handles it just fine.

@chcosta
Copy link
Member

chcosta commented Oct 30, 2018

A special task wouldn't address the "local dev scenario" @dagood brings up. I'm also not sure that a task addresses the fact that most of these packages need to get plugged into the build. It does seem that Arcade could perhaps provide an extensibility point here though which would provide these packages if certain authentication requirements were met. That would allow an official build or a local dev to repro using the same mechanisms. @tmat, thoughts?

@tmat
Copy link
Member

tmat commented Oct 30, 2018

I'm not sure what problem are we trying to solve here. The current solution works well enough. We can probably improve on it in future, once NuGet/Home#6045 is fixed.

@dagood
Copy link
Member

dagood commented Oct 31, 2018

For local dev scenario, I think all I'm asking for is a pointer to a doc.

YAML templating or inclusion in some base template extensibility sounds fine. (Although it doesn't seem like anyone's really asking for that.) A custom-built task doesn't sound very useful though--I don't see what would/could be improved over the four-liner NuGet Restore task tmat's already pointed out in both issues tracking this feature.

@chcosta
Copy link
Member

chcosta commented Oct 31, 2018

I don't think this would be a YAML template thing, I hadn't seen that proposed or even considered it.

@dagood
Copy link
Member

dagood commented Oct 31, 2018

Ah, sorry, that's what I interpreted It does seem that Arcade could perhaps provide an extensibility point here though which would provide these packages if certain authentication requirements were met. to mean--I was thinking too hard about it in context of build steps.

For the Arcade SDK, that would be similar to the BuildTools approach. What I'm thinking @tmat has said is that we shouldn't need that additional tooling for local devs to find this easy to use. (Either way, I'm just hoping for a doc with steps that are straightforward to follow, extra tooling or no.)

@tmat
Copy link
Member

tmat commented Oct 31, 2018

@dagood This is the command to use to restore the internal toolset project:

D:\Roslyn\build\ToolsetPackages>nuget.exe restore InternalToolset.csproj -Source https://devdiv.pkgs.visualstudio.com/_packaging/8f470c7e-ac49-4afe-a6ee-cf784e438b93/nuget/v3/index.json
MSBuild auto-detection: using msbuild version '15.9.14.22190' from 'C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\15.0\bin'.
Restoring packages for D:\Roslyn\build\ToolsetPackages\InternalToolset.csproj...
CredentialProvider.VSS: Getting new credentials for source:https://devdiv.pkgs.visualstudio.com/_packaging/8f470c7e-ac49-4afe-a6ee-cf784e438b93/nuget/v3/index.json, scope:vso.packaging_write vso.drop_write
  GET https://devdiv.pkgs.visualstudio.com/_packaging/8f470c7e-ac49-4afe-a6ee-cf784e438b93/nuget/v3/flat2/microsoft.dotnet.ibcmerge/index.json
  OK https://devdiv.pkgs.visualstudio.com/_packaging/8f470c7e-ac49-4afe-a6ee-cf784e438b93/nuget/v3/flat2/microsoft.dotnet.ibcmerge/index.json 208ms
  GET https://devdiv.pkgs.visualstudio.com/_packaging/8f470c7e-ac49-4afe-a6ee-cf784e438b93/nuget/v3/flat2/microsoft.dotnet.ibcmerge/4.7.2-alpha-00001/microsoft.dotnet.ibcmerge.4.7.2-alpha-00001.nupkg
  OK https://devdiv.pkgs.visualstudio.com/_packaging/8f470c7e-ac49-4afe-a6ee-cf784e438b93/nuget/v3/flat2/microsoft.dotnet.ibcmerge/4.7.2-alpha-00001/microsoft.dotnet.ibcmerge.4.7.2-alpha-00001.nupkg 674ms
Installing Microsoft.DotNet.IBCMerge 4.7.2-alpha-00001.
Committing restore...
Generating MSBuild file D:\Roslyn\build\ToolsetPackages\obj\InternalToolset.csproj.nuget.g.props.
Writing lock file to disk. Path: D:\Roslyn\build\ToolsetPackages\obj\project.assets.json
Restore completed in 7.04 sec for D:\Roslyn\build\ToolsetPackages\InternalToolset.csproj.

NuGet Config files used:
    D:\Roslyn\NuGet.Config
    C:\Users\tomat\AppData\Roaming\NuGet\NuGet.Config
    C:\Program Files (x86)\NuGet\Config\Microsoft.VisualStudio.Offline.config

Feeds used:
    https://devdiv.pkgs.visualstudio.com/_packaging/8f470c7e-ac49-4afe-a6ee-cf784e438b93/nuget/v3/index.json

Installed:
    1 package(s) to D:\Roslyn\build\ToolsetPackages\InternalToolset.csproj

@maririos
Copy link
Member

@tmat is the internal toolset project part of Arcade? or something that will be ported to Arcade that everyone needs to know? Or is it something only Roslyn does.

@dagood it looks like the conversation has drifted from the original ask of the issue. Do you mind opening an issue with your request for a doc?

@weshaggard
Copy link
Member

We should move this conversation to #100.

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

No branches or pull requests

7 participants