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

Added new verbs: powershell_core and powershell #2211

Merged
merged 3 commits into from
Aug 12, 2024

Conversation

ProjectSynchro
Copy link
Contributor

Saw an older PR that died a few years ago so I figured I'd try.

The Powershell Core verb is based on the Latest LTS release that supports both 32 and 64 bit Windows.

I didn't include dotnet48 as it isn't explicitly required by PowerShell Core and is quite heavy to install, it doesn't provide everything you would be using it for anyways. If it's wanted it can trivially be added.

The powershell verb is based on wrapper code is hosted at: https://github.com/ProjectSynchro/powershell-wrapper-for-wine, this is a fork of https://github.com/PietJankbal/powershell-wrapper-for-wine, I contacted the maintainer but they didn't get back to me so I went ahead and forked, if things change the links to the powershell wrapper binaries and profile.ps1 can be changed.

Let me know if there is anything I should do formatting-wise, I kind of copied what was around and attempted to keep things in alphabetical order. I would stick powershell_core after powershell, but with powershell requiring powershell_core it is what it is.

@ProjectSynchro ProjectSynchro marked this pull request as draft April 14, 2024 21:10
@ProjectSynchro ProjectSynchro marked this pull request as ready for review April 14, 2024 21:20
src/winetricks Outdated Show resolved Hide resolved
src/winetricks Outdated Show resolved Hide resolved
src/winetricks Outdated Show resolved Hide resolved
src/winetricks Outdated Show resolved Hide resolved
src/winetricks Outdated Show resolved Hide resolved
@ProjectSynchro
Copy link
Contributor Author

I tried to derive as much as possible from the metadata (including version from file1), let me know if you'd like any more changes.

@mactan-sc
Copy link

looking forward to having a verb for this, the star citizen game launcher is unable to perform two critical functions without powershell

@ProjectSynchro
Copy link
Contributor Author

@austin987
Is there anything else that you'd like done to get this merged?

AFAIK I have actioned everything you had suggested.

Cheers.

src/winetricks Outdated Show resolved Hide resolved
src/winetricks Outdated Show resolved Hide resolved
@ProjectSynchro
Copy link
Contributor Author

I cleaned up the commit history.

Let me know if there are any additional changes needed, from what I can see I think it should be good to go, unless there's another linter I haven't found to run 😄

src/winetricks Outdated Show resolved Hide resolved
src/winetricks Outdated Show resolved Hide resolved
Powershell Core v7.2.x is the Latest LTS release series that supports both 32 and 64 bit Windows.
@ProjectSynchro
Copy link
Contributor Author

Decided to just go with the "silence shellcheck" route, hopefully that's okay. 😄

Ran the same script your CI does to verify scripts and that seems to have passed.

@austin987
Copy link
Contributor

Decided to just go with the "silence shellcheck" route, hopefully that's okay. 😄

Yep, that's fine.

Ran the same script your CI does to verify scripts and that seems to have passed.

Are you sure :)? It fails for GitHub Actions with 0.8.0 (and with 0.10.0 for me locally):

In src/winetricks line 12489:
        w_try cp "${file3}" "${W_PROGRAMW6432_UNIX}/PowerShell/7/${file3}"
                  ^------^ SC2154 (warning): file3 is referenced but not assigned (did you mean 'file'?).

What version of shellcheck did this work with?

AFAIK Shellcheck directives have to be on the line above the offending line (or at the top of the file, or before the function name), see https://www.shellcheck.net/wiki/Directive. That said, to be fair, I'm not sure why it worked for the other cases.

The safest way to handle it is to put the directive directly above the affected lines (that way if any future variables are added to these functions, they're not affected).

@ProjectSynchro
Copy link
Contributor Author

ProjectSynchro commented Aug 9, 2024

Are you sure :)? It fails for GitHub Actions with 0.8.0 (and with 0.10.0 for me locally):

In src/winetricks line 12489:
        w_try cp "${file3}" "${W_PROGRAMW6432_UNIX}/PowerShell/7/${file3}"
                  ^------^ SC2154 (warning): file3 is referenced but not assigned (did you mean 'file'?).

What version of shellcheck did this work with?

This worked with 0.10.0, but after testing this again it looks like the shell-checks script fails silently if bashate is not installed. I'm not sure if this is intentional or not but I'm guessing not :)

Installing it has the script running correctly, and I was able to repro the issue you found.

BTW, sent a fix in here: #2254

AFAIK Shellcheck directives have to be on the line above the offending line (or at the top of the file, or before the function name), see https://www.shellcheck.net/wiki/Directive. That said, to be fair, I'm not sure why it worked for the other cases.

The safest way to handle it is to put the directive directly above the affected lines (that way if any future variables are added to these functions, they're not affected).

Agreed, I suspect since the scripts are interpreted, it disables the checks in whatever context the directive is located. I stuck them above each of the effected lines

@austin987
Copy link
Contributor

Thanks (for the PR, and your patience :) )!

@austin987 austin987 merged commit eeaed1f into Winetricks:master Aug 12, 2024
5 checks passed
@ProjectSynchro
Copy link
Contributor Author

No problem :)

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.

3 participants