-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Allow --releasify
to specify the required .NET framework version
#940
Conversation
Setting IDS_FXVERSION now lets you specify a required .NET Framework version between 4.5 and 4.6.2
@NeilSorensen thanks you for your effort. is this testable ? |
Unfortunately, I couldn't find any tests on any of the code that I changed. The most important change is also probably the least testable, because it depends on resources. If I missed some tests, I'd be happy to modify them if you can point them out to me |
src/Setup/FxHelper.cpp
Outdated
|
||
WCHAR szTempPath[_MAX_PATH]; | ||
DWORD dwTempPathResult = GetTempPath(_MAX_PATH, szTempPath); | ||
|
||
if (dwTempPathResult == 0) { | ||
hr = AtlHresultFromLastError(); | ||
goto out; | ||
} else if (dwTempPathResult > _MAX_PATH) { | ||
} | ||
else if (dwTempPathResult > _MAX_PATH) { |
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.
Move this to the previous line
src/Update/Program.cs
Outdated
@@ -434,8 +436,14 @@ public void Releasify(string package, string targetDir = null, string packagesDi | |||
|
|||
var writeZipToSetup = findExecutable("WriteZipToSetup.exe"); | |||
|
|||
try { | |||
var result = Utility.InvokeProcessAsync(writeZipToSetup, String.Format("\"{0}\" \"{1}\"", targetSetupExe, zipPath), CancellationToken.None).Result; | |||
try |
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.
Please match the existing code style with braces
src/Update/Program.cs
Outdated
try | ||
{ | ||
var arguments = String.Format("\"{0}\" \"{1}\"", targetSetupExe, zipPath); | ||
if (!String.IsNullOrEmpty(frameworkVersion)) |
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 just default this to .NET 4.5?
if (argc == 5 && wcscmp(argv[3], L"--set-required-framework") == 0) { | ||
setFramework = true; | ||
} | ||
else if (argc != 3) { |
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 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'm a little confused by what you'd like here. If --set-required-framework
is unspecified, it will not overwrite the default value of net45, which is set in setup resources. Would you prefer WriteZipToSetup to always overwrite that value? Or are you suggesting making --set-required-framework
a required flag for WriteZipToSetup?
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.
Nevermind, that's a style change...
This seems reasonable, I've marked some style changes but other than that, thanks for the PR |
@NeilSorensen what can we do to get those style changes fixed so this PR can be merged? |
Sorry, life got in the way for a while. I'll get it updated today |
@NeilSorensen Could you update this for .NET 4.7? |
@koliva8245 as I understand it, .NET 4.7 is not yet released for anything other than Windows 10 with the creator's update. Once it hits wide release, it should be fairly simple to add it as well |
I'll try to get this out soon, if I forget please start harassing me on Twitter dot com until I do it :) |
:( |
Thanks so much @NeilSorensen! |
Anyone working on getting .NET 4.7 support for this? |
460798 is a Release number for .NET 4.7 on Windows 10 CU RTM (with latest cumulative updates): Version: 4.7.02046 |
Can someone add docs for this? I recalled this was available, but had to come back to this bug to find out the cmd line switch. |
@dealproc Agreed. I'm not 100% either. Based on looking at the code, it looks like you pass |
Thanks for the tip. I should have checked the console help (above) which has it. |
A proposed solution for #341