Skip to content
This repository has been archived by the owner on Feb 19, 2019. It is now read-only.

[Enhancement] Refresh env vars after Install #158

Closed
wants to merge 1 commit into from

Conversation

Iristyle
Copy link
Contributor

  • Registry keys for USER and MACHINE are examined during the refresh
  • Unit tests in place (but not running right just yet)
  • Special treatment for PATH

@Iristyle
Copy link
Contributor Author

See #153

@rismoney
Copy link
Contributor

@ferventcoder - "don't pull yet - pull requests" - where have i seen that before?

@ferventcoder
Copy link
Contributor

Ha - the don't pull yet is still in the one that is outstanding. :D

- Registry keys for USER and MACHINE are examined during the refresh
- Unit tests in place and pass (as long as running admin)
- Special treatment for PATH to munge MACHINE and USER variants
@Iristyle
Copy link
Contributor Author

This is all ready to go... presuming the code is called in the right spot now.

If .\build.bat is run as admin, the unit tests against MACHINE pass... but if not, those 2 tests will fail. (USER will pass)

Not sure if you have any similar tests that require elevated privileges? I know that you have a helper to run commands elevated.. maybe that's the best option?

@Iristyle
Copy link
Contributor Author

Tried running tests with your *AsAdmin helper.. not so good for this use case. So I guess you'll just have to live with failing tests when you're not admin.

The problem is not refresh from MACHINE -- that doesn't need elevation.. its that I can't write a new environment variable to MACHINE without admin, for the sake of the test.

@mwrock
Copy link
Contributor

mwrock commented Sep 18, 2012

So I've been playing around with this (related to Issue #153 and issue #134). Cleaning up the tests and integration and should have it in a mergeable state tonight. I think this will address scenarios where you are installing a string of packages via normal dependencies or a package config and one ChocolateyInstall may need the new variables defined or modified by a previous install.

Unfortunately I don't think it will have much of an impact on the scenario where the user's shell would like to use the refreshed vars. I had hoped and (incorerrectly I think) assumed that this PR as is would allow someone using chocolatey via PS to have their current shell inherrit all updated env vars and adding a small bit of batch script goodness would expand this to normal command line shells as well. The challenge is that even if you are accessing chocolatey via PS (which I imagine most do), if you use the cinst flavored shortcut commands or even the longer Chocolatey-Install commands, you stil go through a batch shell. This means that when chocolatey calls ths Update-SessionVariables function, the env: affected is a different shell than the one that the user is in. Of course the user can invoke the function on their own and that will update their shell bt it is something they will have to do on their own. Even adding batch script compliance to this will not fix that since again, that is a different shell. That would address users who call chocolatey in a normal command line shell, but is that a typical use case? To me using the batch shell as a day to day shell is like using Windows in Safe Mode which is not a good experience.

One way to get this to possibly get this to work but i think is unacceptable would be to add a line to the user's $profile that modifies their prompt(). This might call any existing prompt, refresh the variables and then erase itself. This fels fragile and maybe even invasive.

@Iristyle
Copy link
Contributor Author

Yeah, agree that modding users PS profile with an add to Update-SessionEnvironment is invasive, so I would rule that out. Some people (like me) use customized prompts, and you would have the possibility of breaking those.

Why not just update how the cinst command is called, so that it calls Update-SessionEnvironment after the batch shell has finished up?

I didn't realize that's how you guys were doing things, but if you're using the PS jobs api, you can just hook the event (if you're not already) for when the job finishes... at which point you can make the call to refresh the session vars.

@ferventcoder
Copy link
Contributor

I'm one of those archaic users that spends more time on the straight command line shell. :(

@Iristyle
Copy link
Contributor Author

It would be a little hacky, but you could have the variables export to a well known temp file location, for import back into cmd, no? Maybe even pipe the output somewhere (not sure if that's feasible)

The only thing that makes me jump to cmd are some of the dir switches I grew up on

dir /s /od

A real verbose PITA in Powershell ;0

@ferventcoder
Copy link
Contributor

you can just call dir in powershell (although it's an alias of something else now).

@Iristyle
Copy link
Contributor Author

OT: But yeah, it's aliased to get-childitem ... where you have -Recurse and -Filter and -Include

I don't know if they fixed that cmdlet is PS3, but -Filter is the faster option, b/c it passes the param to the underlying OS, but it's also busted for anything complex, so you have to do fancier things like -Include 'foo*', 'bar*' ... which can be horrendously slow on larger file sets due to implementation details.

And obviously no way to pass all the switches like /ad -- equiv for Get-ChildItem is roughly | ? { $_.PsIsContainer }

PITA

@mwrock
Copy link
Contributor

mwrock commented Sep 18, 2012

I think getting the vars working on Rob's favorite shell is very possible. But no matter how you change cinst or anything else in the batch goo to get the vars to play nice with the normal command line, it wont work for the user calling cinst from powershell because once yoy are in cinst, you are in a different shell and it cant reach back into the user's PS session.

@ferventcoder
Copy link
Contributor

Maybe that brings up the idea of opening something native to powershell with chocolatey - and installing it to their profile (much like poshgit/hg/whathaveyou do). It still looks like the native command but it doesn't fall down to the command shell.

As in Set-Alias cinst Chocolatey-Install (and for all the rest). :D

@mwrock
Copy link
Contributor

mwrock commented Sep 18, 2012

Hey. That's a good idea. Create a example-profile that also gets dot sourced into ones profile and alias all the commands.

@Iristyle
Copy link
Contributor Author

+1 on that.. if you can get it to work.

Aren't you guys passing a bunch of cmd line switches to powershell.exe (like -ExecutionPolicy). Might run into some issues... I have my machine set to Unrestricted, so I wouldn't have any issues, but not sure about everyone else?

@mwrock
Copy link
Contributor

mwrock commented Sep 18, 2012

I think most that work in PS have that set already since its fairly useless otherwise.

@ferventcoder
Copy link
Contributor

Agreed on what Matt said. And if they haven't, they will find chocolatey fairly useless in powershell as well. :D

@ferventcoder
Copy link
Contributor

The real trick is finding the modules since someone can install it to other folders. We could also provide a module that is dot-sourced with all of the commands we want seen.

@Iristyle
Copy link
Contributor Author

Best to follow PsGet conventions IMHO, so you can just

Import-Module Chocolatey

Each PSM1 uses Export-ModuleMember like this

@ferventcoder
Copy link
Contributor

I'd prefer not documenting the conventions on the methods, even though those could be used as well.

cinst (so it's one for both interfaces) references Install-ChocolateyPackage (or something similar if we have a naming conflict).
cup - Update-ChocolateyPackage
cver - List-ChocolateyVersion (or something like this)
cuninst - Uninstall-ChocolateyPackage

@ferventcoder
Copy link
Contributor

LOL - Export-ModuleMember is exactly what chocolatey already does for the utility functions it allows the chocolateyInstall.ps1 to use.

https://github.com/chocolatey/chocolatey/blob/master/src/helpers/chocolateyInstaller.psm1#L12

@Iristyle
Copy link
Contributor Author

Cool, then you're good to go... (as you can tell, I haven't looked at all the Chocolatey source ;0)

My point re: PsGet ... was along the install dir. Sorry if that wasn't clear. By plopping into the user module dir, then you should be able to just call Import-Module Chocolatey from anywhere... and the exported stuff will just work.

If you want to get super fancy (and I'm not at all serious here), you can have a remote server running all the time, that can just export the commands to a Powershell WinRM session using a one-liner! Have you ever seen how the Exchange cmdlets work?

http://technet.microsoft.com/en-us/magazine/hh750396.aspx

I'm not an admin, but had to help show a co-worker once how to use PS -- first time I had encountered that stuff (the power of Import-PSSession that is). Unfortunately, you can get cmdlets / modules from a remote server, but no way (directly) to push them -- which is why I made this coincidentally -- https://github.com/EastPoint/Midori/blob/master/tools/Remoting.psm1

@mwrock
Copy link
Contributor

mwrock commented Sep 19, 2012

OK. Pulled this in. Regarding the issue of failing tests when not running as admin. I changed the test to check if the user is admin and if not, the test is essentially skipped. I am going to change the Test msbuild target to elevate privileges. This way, running as a non admin user using just "Pester" will skip the test, but when the tests run as a build via ./build, the permisions will elevate and the tests will run.

I will also crete a separate issue for adding the batch style refresh and another the profile stuff discussed above.

@mwrock mwrock closed this Sep 19, 2012
@ferventcoder
Copy link
Contributor

@mwrock Did those separate issues get created?

@mwrock
Copy link
Contributor

mwrock commented Nov 4, 2012

Issue #134 is the placeholder for this I think. I just added a note to this for clarification.


From: "Rob Reynolds" notifications@github.com
Sent: Wednesday, October 31, 2012 11:43 PM
To: "chocolatey/chocolatey" chocolatey@noreply.github.com
Subject: Re: [chocolatey] [Enhancement] Refresh env vars after Install (#158)

@mwrock Did those separate issues get created? -
Reply to this email directly or view it on GitHub.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants