-
Notifications
You must be signed in to change notification settings - Fork 220
Optimize Windows Defender exclusion script with native PowerShell #2930 #2939
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
Conversation
Thank you for this contribution. The changes look good too me and the script indeed looks cleaner. I'm not very familiar with Powershell and that was what I came up with after searching the internet a bit. However, I could confirm that @fdcastel's objection is correct and this script could be even simpler. |
Thanks for your feedback @HannesWell, If @fdcastel approves I can proceed to update the PR...as per your guidance ofcourse! |
Apologies for the delay (I was abroad). I agree with @HannesWell that the script can be simplified even further. @IamLRBA Based on my tests (and I believe @HannesWell can confirm as well), just using the call Add-MpPreference -ExclusionProcess $pathsToExclude should be sufficient. There is no need for any if statements or for instantiating a new |
Thank you for the feedback @fdcastel . You're right upon re-testing with just: |
@fdcastel, I've updated the script to simplify it as suggested. It now directly uses |
Looks good to me! I don’t have a Java setup here to test it out, but the changes seem fine. 🚀 👍🏻 |
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 update.
Unfortunately this currently does not compile and a few points should be adjusted, but I made corresponding remarks below.
Furthermore, please squash your changes into one commit and force push that to your existing branch of this PR. Eventually we want to have this as one commit.
"if($existingExclusions -eq $null) { $existingExclusions = New-Object Collections.Generic.HashSet[String] }", //$NON-NLS-1$ | ||
"$exclusionsToAdd=[Linq.Enumerable]::ToArray([Linq.Enumerable]::Where($exclusions,[Func[object,bool]]{param($ex)!$existingExclusions.Contains($ex)}))", //$NON-NLS-1$ | ||
"if($exclusionsToAdd.Length -gt 0){ Add-MpPreference -" + exclusionType + " $exclusionsToAdd }"); //$NON-NLS-1$ //$NON-NLS-2$ | ||
String excludedPaths = paths.stream().map(Path::toString).map(p -> "'" + p.replace("'", "''") + "'") |
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 you please explain the advantage of using single-quotes (and escaping them) over double-quotes in a powershell script? As far as I can tell double-quotes are not allowed in Windows paths and therefore don't have to be escaped.
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 had initially done this to handle the rare case of paths containing single quotes (which are technically valid in Windows paths)
However, as you noted, that might have just added unnecessary complexity.
@@ -342,14 +342,12 @@ public static String createAddExclusionsPowershellCommand(String extraSeparator) | |||
// | |||
// For .NET's stream API called LINQ see: | |||
// https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable |
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 entire comment block (starting at its first line) can be replaced by the following:
// For detailed explanations about how to add new exclusions see:
// https://learn.microsoft.com/en-us/powershell/module/defender/add-mppreference?view=windowsserver2019-ps
.../org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/WindowsDefenderConfigurator.java
Outdated
Show resolved
Hide resolved
.../org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/WindowsDefenderConfigurator.java
Outdated
Show resolved
Hide resolved
@HannesWell, Thanks so much for the corrections and changes suggested. |
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.
Thank you for the update and sorry for the delayed response.
The code change is mostly fine, just two remarks below.
After you have applied them, please locally squash all changes you made into your first commit and update the commit message if necessary to cover this change as a whole. Then fetch the upstream master and rebase on top of it. After that please force push the updated commit to this PR's branch so that eventually it just contains one commit.
Thank you :)
.../org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/WindowsDefenderConfigurator.java
Outdated
Show resolved
Hide resolved
.../org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/WindowsDefenderConfigurator.java
Outdated
Show resolved
Hide resolved
b1b238b
to
45a70fd
Compare
@HannesWell, I think I have successfully squashed the commits into one |
Since the script to add an exclusion to Windows Defender seems overly complex, this script replaces the complex LINQ-based exclusion check with a more idiomatic and simpler version PowerShell implementation. Fixes eclipse-platform#2930
00ce0a9
to
43c3a07
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.
I think I have successfully squashed the commits into one
Yes you did, thanks for that. But the commit message needed a little tweaking.
Besides that I also applied my two pending requests and now this is ready for submission.
Thank you for this contribution to simplify the code. That's more than welcome.
Test Results 2 778 files ±0 2 778 suites ±0 1h 31m 21s ⏱️ - 12m 50s For more details on these failures, see this check. Results for commit 43c3a07. ± Comparison against base commit 477d106. |
Thank you for the opportunity @HannesWell , You were a very helpful mentor! |
cc
@HannesWell
Hello,
Since the script as per @fdcastel, to add an exclusion to Windows Defender seems overly complex. Based on your concerns about the proposed
Add-MpPreference -ExclusionProcess 'C:\Program Files\DBeaver\dbeaver.exe'
script, this script replaces the original complex LINQ-based exclusion check with a more idiomatic and simpler version PowerShell implementation.How It Addresses the your Concerns:
Fixes #2930