Skip to content
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

nmap: Extract and install npcap #260

Closed
wants to merge 7 commits into from
Closed

nmap: Extract and install npcap #260

wants to merge 7 commits into from

Conversation

linsui
Copy link
Contributor

@linsui linsui commented Jul 23, 2019

Now npcap will be installed in nmap directory. Due to unknown reasons, uninstall will fail and need to uninstall manaully.

Can be merged after ScoopInstaller/Scoop#3547

bucket/nmap.json Outdated Show resolved Hide resolved
@linsui
Copy link
Contributor Author

linsui commented Jul 24, 2019

The uninstall need privileges. nmap uninstaller won't uninstall npcap so there is no problem before. By the way, could anyone have a look at ScoopInstaller/Scoop#3567 ? With sudo it works. Will it be fixed or we need to use sudo instead?

@rasa
Copy link
Member

rasa commented Jul 24, 2019

In my opinion, if an app needs elevation to install/uninstall, it should depend on sudo, and then use it to install the app. See https://github.com/lukesampson/scoop-extras/blob/e73c63bfbe3f369a126c4c74fd9baf8e7ff08281/bucket/pritunl-client.json#L16 for an example.

@niheaven
Copy link
Member

See ScoopInstaller/Extras#2464, you could use -RunAs.

@linsui
Copy link
Contributor Author

linsui commented Jul 25, 2019

@rasa @niheaven Thanks. What's the suggested way?

@niheaven
Copy link
Member

I suggest -RunAs that needn't depends: sudo, and please check if works.

But this param was fixed in ScoopInstaller/Scoop#3547 and not be merged into scoop/master yet, so you might halt your PR and wait for merging (has been merged into develop)

@linsui
Copy link
Contributor Author

linsui commented Jul 25, 2019

@niheaven It works well, thanks. If it can be write as

    {
        "file": "Uninstall.exe",
        "args": "/S",
        "admin": true
    }

it would be better 😂

@niheaven
Copy link
Member

Wow, right, I'll do this works in ScoopInstaller/Scoop#3502

@linsui
Copy link
Contributor Author

linsui commented Oct 24, 2019

Please review.

Copy link
Contributor

@Ash258 Ash258 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think that RunAs usage is good for Main bucket. Will there be UAC popup? Will it fail if no administrator terminal / sudo is not used?

bucket/nmap.json Outdated
"/S",
"/D=$dir"
]
"script": "Invoke-ExternalCommand -FilePath \"$dir\\npcap.exe\" -ArgumentList \"/S /D=$dir\\npcap\" -RunAs | Out-Null"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unescaped dir after /D

Suggested change
"script": "Invoke-ExternalCommand -FilePath \"$dir\\npcap.exe\" -ArgumentList \"/S /D=$dir\\npcap\" -RunAs | Out-Null"
"script": "Invoke-ExternalCommand -FilePath \"$dir\\npcap.exe\" -ArgumentList \"/S /D=\"\"$dir\\npcap\"\"\" -RunAs | Out-Null"

Try installation with single quotes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work with quotes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nsis requires /D$dir. It breaks with quotes. See ScoopInstaller/Scoop#2003

bucket/nmap.json Outdated Show resolved Hide resolved
linsui and others added 2 commits October 24, 2019 10:43
Co-Authored-By: Jakub Čábera <cabera.jakub@gmail.com>
@linsui
Copy link
Contributor Author

linsui commented Nov 3, 2019

@Ash258 RunAs shows a UAC pop.
Npcap need privelege to install so if maintainers want to keep main bucket non-privilege we can move it to extras or non-portable. It seems that the only criteria which is executed strictly now is non-GUI...

@linsui linsui closed this Oct 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants