-
Notifications
You must be signed in to change notification settings - Fork 176
Convert script and directory _nsis.py subcommands into NSIS code
#1069
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
|
@marcoesters Is this something that is awaiting review? Let me know and I can review it. |
You are free to leave comments, but in the end, I need approval from a |
| call un.AbortRetryNSExecWait | ||
| ${EndIf} | ||
| !insertmacro AbortRetryNSExecWaitLibNsisCmd "rmpath" | ||
| !insertmacro AbortRetryNSExecWaitLibNsisCmd "rmreg" |
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.
Why is this no longer needed?
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.
It was actually never needed. The uninstallation via conda-standalone already performs the same function during the installation process. I included that line back then by mistake.
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 see, thanks!
Co-authored-by: Robin <[email protected]>
jaimergp
left a comment
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.
Nice cleanups! Thanks!
|
I found a way to remove the |
|
Using |
| ${EndIf} | ||
| !insertmacro AbortRetryNSExecWaitLibNsisCmd "rmpath" | ||
| {%- if has_conda %} | ||
| ${If} ${FileExists} "$INSTDIR\.nonadmin" |
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.
Optional:
In the Function un.onInit we also have this logic:
${If} ${FileExists} "$INSTDIR\.nonadmin"
SetShellVarContext Current
${Else}
SetShellVarContext All
${EndIf}
If you prefer, you can create a new variable (maybe un.IsUserInstall?) and assign that variable within that function, to reduce the number of if .nonadmin exists.
I have no strong opinion though, I also like the simplicity with simply using $R0 as in your commit.
| # that DOSKEY (called by conda_hook.bat) is not a valid command. | ||
| # While the operation still succeeds, this error is confusing. | ||
| # Calling via cmd.exe fixes that. | ||
| push '"$CMD_EXE" /D /C "$INSTDIR\condabin\conda.bat" init cmd.exe --reverse --$R0' |
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.
| push '"$CMD_EXE" /D /C "$INSTDIR\condabin\conda.bat" init cmd.exe --reverse --$R0' | |
| push '"$CMD_EXE" /D /C "$INSTDIR\condabin\conda.bat" init --reverse --$R0' |
Should we revert for both cmd and powershell (which is achieved by passing no positional arguments)?
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.
The powershell function does not perform path validation, so it will also revert profiles that point to a different installation. The cmd.exe function, on the other hand, only reverts the autorun entry that belongs to the installation.
Description
Convert simple script and directory subcommands into NSIS code to further decouple the python dependency from EXE installers (#549). Scripts are already executed directly via
cmd.exefor pre-install scripts andmkdirsis a simple directory creation command.Note that this means that
post-installandpre-uninstallscript failures will cause the installation/uninstallation process to fail now. I think this is good behavior as it has masked issues in the past (see #942 and #1020).Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?