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.

Sign up for GitHub

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

Jump to bottom

Workload + Sdk version Constraints #4753

Merged
merged 12 commits into from
Jun 1, 2022
Merged

Workload + Sdk version Constraints #4753

merged 12 commits into from
Jun 1, 2022

Conversation

JanKrivanek
Copy link
Member

@JanKrivanek JanKrivanek commented May 17, 2022 •
edited
Loading

Problem

#3107
Complement SDK changes: dotnet/sdk#25505

Solution

  • Extracted some common functionality
  • Moved Abstractions to own namespace
  • Added Abstractions needed for Workload and SdkVersion constraints (to be injected by host)
  • Workload Constraint
  • Sdk Version Constraint

TBD:

  • Unit tests

Checks:

  • Added unit tests
  • Added #nullable enable to all the modified files ?

@JanKrivanek JanKrivanek requested a review from a team as a code owner May 17, 2022 18:05
phenning
phenning reviewed May 17, 2022
View reviewed changes
src/Microsoft.TemplateEngine.Abstractions/Constraints/IWorkloadsInfoProvider.cs Outdated
/// <summary>
/// Set of installed workloads.
/// </summary>
public IEnumerable<WorkloadInfo> InstalledWorkloads { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

How will you be implementing this in the CLI host? Does the Template Locator have an API surface that we can call from VS to pass this to the template engine? Currently VS knows nothing about installed SDK workloads, we only grab the list of packages from installed workloads from the template locator.

Copy link
Member Author

@JanKrivanek JanKrivanek May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm going to open PR in SDK during today/tomorrw and I'll link it here. dotnet/sdk#25505

SDK has VisualStudioWorkloads.GetInstalledWorkloads that does this (and I'll be utilizing it in my helper) - however it is used only for Windows platform. I was hoping to get from you some knowledge or PoCs with knowledge about situation on Mac.

Another question is how to handle VS instances with other older SDK versions:
a) backport the utility code (not sure if it's a good idea at all)
b) inject custom provider that utilizes sdk workload list or any available API
c) not support the Workload constraints (same will happen by simply not passig the provider from host)

Let's have a discussion about this topic

@JanKrivanek JanKrivanek changed the title Constraint workflows Workload + Sdk version Constraints May 18, 2022
vlada-shubina
vlada-shubina requested changes May 18, 2022
View reviewed changes
Copy link
Member

@vlada-shubina vlada-shubina left a comment

Choose a reason for hiding this comment

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

Mostly questions, some concerns around namespaces, imo constraints logic may be simplified.
Consider Async for new components.

@JanKrivanek JanKrivanek mentioned this pull request May 18, 2022
Merged
@JanKrivanek JanKrivanek requested a review from vlada-shubina May 19, 2022 21:04
@JanKrivanek JanKrivanek requested a review from phenning May 26, 2022 16:00
vlada-shubina
vlada-shubina requested changes May 27, 2022
View reviewed changes
Copy link
Member

@vlada-shubina vlada-shubina left a comment

Choose a reason for hiding this comment

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

Please review CTAs as the seems not be applicable to Visual Studio (or potentially other host).

@JanKrivanek JanKrivanek requested a review from vlada-shubina May 30, 2022 10:04
vlada-shubina
vlada-shubina approved these changes May 31, 2022
View reviewed changes
Copy link
Member

@vlada-shubina vlada-shubina left a comment

Choose a reason for hiding this comment

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

Looks good, couple of comments inline.

src/Microsoft.TemplateEngine.Edge/Constraints/SdkVersionConstraintFactory.cs
Comment on lines +116 to +121
string version = await providers[0].GetCurrentVersionAsync(cancellationToken).ConfigureAwait(false);
NuGetVersionSpecification currentSdkVersion = ParseVersion(version);

cancellationToken.ThrowIfCancellationRequested();
IEnumerable<NuGetVersionSpecification> versions = (await providers[0].GetInstalledVersionsAsync(cancellationToken).ConfigureAwait(false)).Select(ParseVersion);

Copy link
Member

Choose a reason for hiding this comment

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

may be done in parallel

Copy link
Member Author

Choose a reason for hiding this comment

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

Due to targetting netstandard 2.0 we do not have IAsyncEnumerable here, but rather Task<IEnumerable<string>> (and just a single call to single provider) - so there is no space for further paralelization. (calling ParseVersion on each version string in enumeration in parallel would have more overhead that doing it serially - as we likely cannot expect this to return more than few elements)

test/Microsoft.TemplateEngine.Edge.UnitTests/SdkVersionConstraintTests.cs

// This is a workaround in a weird bug with FakeItEasy, when using:
// ISdkInfoProvider sdkInfoProvider = A.Fake<ISdkInfoProvider>();
// A.CallTo(() => sdkInfoProvider.GetVersionAsync(A<CancellationToken>._)).Returns(Task.FromResult(sdkVersion));
Copy link
Member

@vlada-shubina vlada-shubina May 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Did you also try A.CallTo(() => sdkInfoProvider.GetVersionAsync(default)).Returns(sdkVersion)
https://fakeiteasy.readthedocs.io/en/stable/faking-async-methods/
I think the issue might be in trying to "fake" cancellation token, but badly reported; I'm not sure if faking structs is supported.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - unfortunately didn't help. CancellationToken is not actually being faked here - the Task is. In some random cases the Task was returning empty string within the tested method (not happening if I create mock manually)

@JanKrivanek JanKrivanek enabled auto-merge (squash) June 1, 2022 05:34
@JanKrivanek JanKrivanek merged commit 9582b36 into dotnet:main Jun 1, 2022
@JanKrivanek JanKrivanek deleted the constraint-workflows branch June 1, 2022 07:22
@JanKrivanek JanKrivanek mentioned this pull request Jun 8, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vlada-shubina vlada-shubina vlada-shubina approved these changes

@phenning phenning Awaiting requested review from phenning

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JanKrivanek @phenning @vlada-shubina