(vlc) Added portable package as stream #909
Conversation
majkinetor
added
Requires chocolatey-community User
| packageName = 'vlc' | ||
| fileType = 'exe' | ||
| file = $installerFile | ||
| file = gi $toolsPath\*-win32.exe | ||
| file64 = gi $toolsPath\*-win64.exe |
There was a problem hiding this comment.
Aliases need replaced with full cmdlet name.
| packageName = 'vlc' | ||
| fileType = 'exe' | ||
| file = $installerFile | ||
| file = gi $toolsPath\*-win32.exe |
| silentArgs = '/S' | ||
| validExitCodes = @(0, 1223) | ||
| } | ||
| Install-ChocolateyInstallPackage @packageArgs | ||
| rm ($toolsDir + '\*.' + $packageArgs.fileType) |
|
|
||
| $packageArgsZip = @{ | ||
| PackageName = 'vlc.portable' | ||
| FileFullPath = gi $toolsPath\*-win32.zip |
| $packageArgsZip = @{ | ||
| PackageName = 'vlc.portable' | ||
| FileFullPath = gi $toolsPath\*-win32.zip | ||
| FileFullPath64 = gi $toolsPath\*-win64.zip |
| if ($isExe) { | ||
| Install-ChocolateyInstallPackage @packageArgsExe } | ||
| else { | ||
| ls $toolsPath\* | ? { $_.PSISContainer } | rm -Recurse -Force |
There was a problem hiding this comment.
Replace ls and rm with full cmdlet name.
| ls $toolsPath\* | ? { $_.PSISContainer } | rm -Recurse -Force | ||
| Get-ChocolateyUnzip @packageArgsZip | ||
| } | ||
| ls $toolsPath\*.$filetype | % { rm $_ -ea 0; if (Test-Path $_) { sc "$_.ignore" } } |
There was a problem hiding this comment.
Replace aliases with full cmdlet name.
| @@ -23,6 +35,8 @@ if ($pp.Language) { | |||
| sp HKCU:\Software\VideoLAN\VLC Lang $pp.Language | |||
There was a problem hiding this comment.
Should be Set-ItemProperty rather than alias.
|
Per 1.1.7 default aliases are allowed. We already had this discussion number of times. |
|
As per a previous PR (for peazip) comment, 1.1.7 states: Use aliases sparingly. If you use them, limit their use to the default aliases which are read-only (can not be modified by the user), well known, and will be supported in the future versions of Powershell. Also ensure they are compatible with PowerShell v2+. (my emphasis) 1,1.7 doesn't allow the allow the use of aliases that are not read-only. |
|
I will change that to reference 'default' aliases and remove all other notes which apparently only confuse people. It doesn't make any sense that I am allowed to use |
| Destination = $toolsPath | ||
| } | ||
|
|
||
| if ($isExe) { |
There was a problem hiding this comment.
I'm a little sceptical of having both the logic for portable and install packages in a single script, may cause confusion from some users.
Would be better to copy a file during update from a location not saved in the nupkg file IMO.
There was a problem hiding this comment.
It makes the package easier to make and maintain which is always good in my book. Users wont complain if it works. I considered first approach you mentioned but it is more confusing from the maintainer perspective. Since this is a PoC for portable/installer streams we may want to see if there is any reaction to this approach in community given this is hi profile package.
There was a problem hiding this comment.
Not that I have strong feelings about it. Feel free to modify the code if you want.
There was a problem hiding this comment.
I agree with @AdmiringWorm in that the logic for both packages being in a single script.
I don't understand how it makes it easier to maintain, or makes it so much more easier to maintain that it offsets the potential confusion, when most packages are maintenance free (that is of course assuming that the underlying software doesn't change how it needs to be installed which requires script changes)?
|
I wholly disagree with changing the documenation to fit the code style. The documentation is there to give everybody guidelines on what aliases should and should not be used. It's not confusing. It's very clear that non read-only aliases should not be used. They shouldl be used as:
It doesn't make any sense that I am allowed to use ri which nobody knows instead rm which everybody knows. Similar ls versus Get-ChildItem. I'm not sure I understand this argument. You don't use aliases becasue people know what they are. The alias system is there to allow you to have shortcuts to your faviourite commands. The alias system is there to be used by the end user. But there are two issues you bring up. The first is something running on the end user device in a known environment. The other is why you write scripts.
The reason the documenation (I'm assuming) says don't use non read-only aliases is that you do not know what they are on the end user device. The non read-only alias system is not a known environment. The read-only alias system IS a known environment because you can guarantee (as much as you can guarantee anything on an end user device) that When I write scripts I don't use any aliases. That's my preference. However using the read-only ones is as close as you can get to that. And to be clear it's the non read-only ones that I listed in the review to be changed. Using the non read-only ones is asking for trouble. And when scripts start breaking on the end users system and they start complaining what are you going to tell them? That they amended their aliases which they are allowed to, and encouraged, to do? All that happens there is that the reliability of Chocolatey packages that you maintain starts to disappear as people stop trusting your packages. And as a result Chocolatey will suffer too. The onyl other solution I can see is to update the System Requirements for Chocolatey to include that people must use all of the aliases that come with PowerShell, unmodified. |
I consider myself master in Powershell and I never knew this. What do you think will be reaction of general user ?
You propopose that we change 200+ packages here to fit the documentation ?
Its not, its a coding style.
MS gives suggestion, not an obligation.
Its because you didn't use correct way, via proxy functions. Your system will break bunch of stuff without them.
This is your interpretation of alias system. Mine is that it reduces cognitive load.
Since I wrote almost the complete guidelines, I am remember how it happened - we assumed default aliases are all readonly.
You can redefine read-only aliases too AFAIK. CBB to find the link now.
That is retoric, not a fact. Here is the fact: we didn't have any problems with aliases so far, there was only a single report for non existing alias once during last year.
I think reliability of packages in this repository are leaps and bound over what we had a year before. Embedding and automation improves reliability the most, not the aliases or any other stuff. Anyway, there are a lot of things to be done to improve packages. More automation, more helper functions, more testing etc. Aliases are the last thing we should discuss. |
|
Besides all that I said here, making ALL packages here non-aliased and pushing fix version is basically trivial (expand aliases recursively and force update ), because we have a good system in place. |
I'm not sure what that adds. I have been working in IT now for 25 years and wouldn't call myself a master. Or an expert. There is always something to learn in development or IT as it's always evolving. You're a master one day and a student the next.
I'm not sure I understand. Reaction to what?
No I propose that as we move forward that you remove aliases from your scripts. Just because it hasn't been done that way in the past doesn't mean you have to continue doing it that way. Do you propose to continue to use aliases that could potentially cause problems?
That's just the first ones that come up. Some are old but still apply. I'm sure there are many many more. PSScriptAnalyzer will flag you using aliases in scripts using the default rules. Microsoft maintains PSScriptAnalyzer now. Visual Studio Code PowerShell extension, by default, flags the use of aliases and you can't submit scripts to the PowerShell Gallery using aliases without them being flagged. That says enough I think.
My system doesn't break anything. I have no issues with anything, with the exception of your AU module which also uses aliases (I don't want to drag that into the discussion just making it clear that there are no issues except with one). If I had issues I'd revert back. Aliases are not for you to use. They are for me to use, change, customise, edit, remove and generally do as I want with them. That's why some are read-only (which I cant change) and some are not read-only (which I can). If aliases were to be used for distributed scripts they woudl make them all read-only so no matter which system you ran it on it would run as expected.
It's very far from my interpretation from the alias system. It's the way it was designed. But you are right, it does reduce cognitive load and that's why they are for use on the command line. You know your environment. If you have access to the command line on another system you know the environment on there so you can using aliases without any issue. If you control the end system you know what aliases are on there too (for example at work you mnight have all of the end user devices have a particular set of aliases). But when you are distributing it to the wider world you don't have that control and you have no idea what those non read-only aliases map to.
They're not. Here are the red only ones
I'm sure you can. That's what I was hinting at. You can also redefine cmdlets. The point is that the alias system has read-only and non read-only aliases. If Microsoft didn't want you to change ANY aliases they'd have made them all read-only. If they enocuraged you to change any alias they'd have made none of them read-only. But they didn't. They made some read-only and some non read-only. So they give you the option of customising some aliases. If you go ahead and go to all the trouble of overriding what they have given you by saying 'hey these are read-only for a reason, but I want to change them anyway' then you are going out of your way to break what they have given you. So you are going to break something. The same is said of overwriting It's common sense. Customise what is intended to be customised (non read-only aliases) and leave the rest alone as you know you are not meant to do that (non read-only aliases and core cmdlets).
It's fact. Your AU module breaks on my system because it uses aliases. Nothing else EVER has broken on my system except that. I have 97 modules installed - 3 downloaded from Github, 6 installed by Choco or came with Windows 10, 12 downloaded from the PowerShell Gallery and the rest core modules. Every single one of them works without issues.
That's like saying I've never crashed my car so there is no need for me to wear a seat belt or get insurance. Non read-only aliases have the potential to cause problems which I have outlined. Just because it's nto been reported doesn't mean you shoudln't fix this issue. You don't get new tyres for the car when you have a blow out, you get them when it's pointed out to you that they have the potential to cuase you problems (ie. they are bald). You've had at least 2 issues with AU (I know I mention it again but I'm trying to draw the distinction with aliases in both) that I have seen with regards to the use of aliases, one from me. Here is also a fact: the use of aliases in scripts are considered bad practice by every single person I have ever spoken with, communicated with, know or respect in the PS community. They have the potential to cause issues which I have highlighted.
Your missing the point entirely. You can't just point to something and say 'thats good' and ignore what's not. Otherwise nothing will evolve. The only reason that the embedding and automation was introduced is because somebody looked at what was there at the time and thought they could improve upon it. Do you stop improving the packages now? Do we not do any more evolution in the package design, moderation or checking?
I absolutely disagree with you. I appreciate that you don't want to discuss them but that doesn't mean they shouldn't be. Not using aliases in packages is a quick fix. Just stop using them. And when future packages are amended, remove them. Install scripts are generally small so it will take seconds, perhaps a couple of minutes tops. And as it is a continuous process it won't add much to it. I really do not understand your resistance to this. If something has the potential to cause the project problems, whether that potential hasn't been realised yet, or whether it's small or huge, it should be tackled. You weigh up the amount of work involved in tackling the issue with the risk and the gain. The amount of work to change new packages as you go along is tiny.
That's not what I was suggesting - what I was suggesting is amending packages as we go along. But if it's trivial to do all packages then that's a win win. However pushing 200+ packages into the queue at one time would be a bad idea as it's going to create a backlog. I would suggest it would be better to update the files that are needed and then have them push the changed files on the next natural push update? |
|
I'm not going to say anything about aliases, so you two can discuss that without me. Anyways, @majkinetor don't forget to remove the upstream packages from Robs repo |
AdmiringWorm
added
migration
|
vlc.portable isn't on robs repo as far as I can see. |
|
hmm, odd. Well, I guess I'll add the choco user then and merge this one in |
AdmiringWorm
removed
Requires chocolatey-community User
|
no wait, it's still the meta package that still needs to be considered before we can start merge. |
|
I considered the metapackage and and didn't do it because the current package is not meta. I will not work on this package any more, I personally do not think vlc portable is useful at all, I just did it to test streams PoC which wasn't well accepted anyway. |
closes #908