-
-
Notifications
You must be signed in to change notification settings - Fork 95
vsdownload: Add option switches for each component #162
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
base: master
Are you sure you want to change the base?
Conversation
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 working on this! This change feels quite complicated, but I think I managed to understand most of what of the changes do here.
I think the main potentially controversial bit here is changing the defaults... Changing the default for the DIA SDK probably is fine, possibly for MSBuild too. But I think it may be worthwhile to keep ATL installed by default.
6c8b8fe
to
d4f771e
Compare
@mstorsjo I pushed a brand new implementation, and updated PR description. Kindly review. |
d4f771e
to
7c063c7
Compare
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 PR, this looks quite good overall I think.
I wonder if it would be good to add some tests for this package selection logic. This should be possible to do quite easily and quickly in the same way as the example I showed in #193 (comment); something like ./vsdownload.py --accept-license --print-selection --msvc-version 17.11 | grep Win..SDK
. With that, it should be possible to show and test concrete cases for various combinations of options, that we do end up installing roughly the expected packages. (For historical --msvc-version
we should be able to check that we do get e.g. a close enough match for the packages, and e.g. no extra SDK installed. For potential future versions we'd probably want to check more loosely, that we do get e.g. correct looking compiler packages, and check e.g. that if we request omitting something, that we don't select those packages.)
Side note; AFAIK the next major MSVS version, 18, should be coming in preview sometime in the end of this year. Hopefully it doesn't deviate too much from the package naming style of 16/17 :-)
include_optional = args.include_optional | ||
skip_recommended = args.skip_recommended | ||
|
||
args.package = ["Microsoft.VisualStudio.Workload.VCTools"] |
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.
Can we add some comments here, e.g. "Find which SDK the workload would include, even if we might not be installing the whole workload in itself"? Plus maybe "back up, temporarily overwrite the args.* variables and restore it, for a call to getSelectedPackages" to clarify what's going on here.
# If no packages are selected, install these versionless packages, which | ||
# gives the latest/recommended version for the current manifest. | ||
defaultPackages = ["Microsoft.VisualStudio.Workload.VCTools"] | ||
defaultPackages, args.package = args.package, [] | ||
defaultIgnores, args.ignore = args.ignore, [] |
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.
So here we're flipping between defaultPackages
and args.package
, to allow using appendPackageSelection
, for filling in data in defaultPackages
and defaultIgnores
, while appendPackageSelection
only touches the args.*
variables. It feels a bit confusing at first, but may be ok with a comment.
if "x86" in args.architecture or "x64" in args.architecture: | ||
defaultPackages.append("Microsoft.VisualStudio.Component.VC.ATL") | ||
appendPackageSelection(args, args.with_msvc, "Microsoft.VisualStudio.Component.VC.Tools.x86.x64") | ||
appendPackageSelection(args, args.with_asan, "Microsoft.VisualCpp.ASAN.X86") |
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.
If I understand the patch correctly, this ASAN package, and the x86.64 tools above, are packages which previously got pulled in automatically through Workload.VCTools
above. Now in a total default run, vsdownload.py --dest /opt/msvc --accept-license
, we'd get them explicitly pulled in both implicitly via the workload and explicitly - is that correct?
In that case, do I understand correctly, that we also could make --with-workload
default to false, to reduce the default install size, as we now do explicitly select the individual packages that we do need? (I guess we could do that as a separate commit then.)
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.
Yes, you are correct. Making --with-workload
default to false is the goal, but it need more tests to see if any more packages are needed.
That would be good, I think. Let me draft one to see if it is valuable. A basic one need to be available before this PR then. |
This PR introduces
--with-<component>
for each component, and also--with-default
as a shorthand to set all--with-<component>
at once.--with-*
has three possible values:All
--with-*
have proper defaults, so that the selected packages are same with previous, for every combinations of command line optionsmsvc-version
,sdk-version
andpackage
. And we get finer-grained control over interested components by explicit--with-<component>
options ,msvc-version
sdk-version
package
True
+
None
a.b
*
None
a.b
c.d.e
*
None
c.d.e
True
c.d.e
+
None
By
--with-default no
, it is achievable now to download a single Windows SDK package only.msvc-version
sdk-version
package
c.d.e
Some interested components do have
D -> I
change, it means, previously the component was included through dependencies, but now it is explicitly included. With these changes, it is possible to exclude workload to reduce download size, but we still get all interested components by default.Note:
I: Explicitly included
X: Explicitly excluded
D: Included through dependencies
N: Not explicitly included but might be included through dependencies