Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add experimental feature support for enabling Windows Feature dependencies #3005
Add experimental feature support for enabling Windows Feature dependencies #3005
Changes from 9 commits
a6ec947
cea7e0d
8000470
f66dd54
ba74875
6d8ca3e
5d048d4
98b44d7
8ebe76b
d59eb0e
8c8a7cf
d4d9430
a4e0eee
b5b2ae4
61a4143
3ba134a
f23ccb7
6711502
427cc0a
c28f5e4
fa20945
fb1a034
0e027e8
09b1a0f
12c4cf5
e1dd564
fdc86bb
918875f
0f36ef9
13200c3
db3c8ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this already handled by
HasAnyOf
? If not, should it be?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.
Installer dependencies are known before this workflow is ever executed; Specific dependency types are also checked for in the
ReportDependencies
workflow.I'm wondering, now that multiple dependency types will be supported, if it makes more sense to set an execution flag for "Contains Package Dependencies" or "Contains Windows Features Dependencies" in the ReportDependencies flow.
With the flags, then would be possible to have a separate workflow around
CheckSupportForWindowsFeatures
that checks the flag ensures the experimental feature is enabled and user is running as admin, to avoid creating copies of the dependencies data in memory unnecessarilyThis comment was marked as resolved.
Sorry, something went wrong.
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.
no longer create vector to aggregate missing dependencies, they will be reported at the end after each windows feature is processed
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Changed to report each feature that failed to enable
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.
@denelon - I may be asking the wrong (scope creep) questions here, so forgive me, but would we want any sort of telemetry around these?
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 think we should be showing each feature as its own item and not aggregating them as well.
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.
Changed so that each windows feature is represented as its own item.
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.
Sounds good; I'm presuming any telemetry needs can be added once support is no longer experimental
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 think this neglects the case where one feature fails to enable, and another has a reboot required. I don't know if we care about that case or not, or if that's even a possibility with the way
hr
gets handled.If
--force
isn't present, I don't think it matters, since both the reboot and the fail would cause the context to terminate, and the termination reason doesn't make much of a difference. If--force
is present, the user would only be informed they are bypassing the failed install, and they wouldn't be informed they are also bypassing the reboot requirement.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.
This whole thing doesn't seem like the right way to handle dependencies. At least, "download" is not consistent with "and also, handle getting all dependencies squared away". So at least maybe change the name of this task?
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.
Moved this out of "download" to the step right before
InstallPackageInstaller
.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.
In reality, this is a larger issue than your feature work, and I think it really needs to be addressed before we can enable dependencies on the whole. Neither of us considered that in retrospect.
You should move the workflow task back here (for now); and probably put it before package dependencies. I say that because one of the packages may actually be dependent on the Windows feature (although it really should have declared it). But maybe it is an optional dependency that affects the install time behavior. It is not likely the case that the Windows feature is dependent on any package though.
The issue is that multiple "downloads" are currently allowed via COM, but if dependencies are actually installed as part of "download", then multiple installs are allowed concurrently. So either we need to change the download/install phase split to download everything, then install everything, or we need to support going back and forth between download and install phases as we handle each dependency.
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.
Thanks for the explanation, I didn't know that COM allowed for multiple downloads. I moved the task back to the suggested location and added a TODO comment explaining the issue that needs to be addressed.
This comment was marked as resolved.
Sorry, something went wrong.
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.
There is a symbolic name and a friendly (localized) name. I don't know how easy it is to get the friendly name from the symbolic name. Which do you expect to see (or both)?
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 would think either would work; If it's easy to get the friendly name, that would be great, but I would expect most users could google the symbolic name to find the friendly name
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.
Changed to showing friendly name as well as the symbolic name such as
[netfx3]