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

Logging is broken in some packages due to new TEMP directory #813

Closed
ferventcoder opened this issue Jun 18, 2016 · 8 comments
Closed

Logging is broken in some packages due to new TEMP directory #813

ferventcoder opened this issue Jun 18, 2016 · 8 comments

Comments

@ferventcoder
Copy link
Member

Due to #590, specifically a9519b5, this is now causing issues when someone has setup an MSI logging file with $env:TEMP\chocolatey\packagename\packagename.msi.log. We need to handle that situation.

@marcinbojko
Copy link

For most of our popular packages, I've had to set $logfile to "$env:TEMP\chocolatey$($packageName).MsiInstall.log"
Works as a workaround.

@jberezanski
Copy link

Most native installers create log files simply as $Env:TEMP\<application>.log (and that is, for example, where I typically look for the log file when I need to troubleshoot an installation). I suggest we put that as a recommendation for package authors to follow.

As for packages using TEMP\chocolatey (or TEMP\chocolatey), correct me if I'm wrong, but have those directories ever been actually documented? I believe they have not and packages putting log files there should be treated as relying on non-contractual implementation details. The package validator (or verifier? the one that performs the actual installation - I always confuse the two) could actually detect this and flag it as package error.

To mitigate this situation, after performing the installation/uninstallation of a package, Chocolatey could scan the TEMP\chocolatey directory and move all .log and .txt files it finds to TEMP, with a warning.

@ferventcoder
Copy link
Member Author

Unfortunately choco new generated out the default silent args with this -
that's why you see it done in this way.

I disagree with the assessment to move the files with a warning - the
$env:TEMP value in packages comes from cacheLocation if it is set and from
$env:TEMP (actual) if it is not set. Some organizations disallow the use of
the TEMP folder, so moving it from where the location it drops to would
actually be harmful.

The better service is to ensure the folder exists, overcoming a limitation
in the installer technology. This makes the most sense for Chocolatey
design, making hard things simpler.

And the Chocolatey subfolder of $env:TEMP (actual) is guaranteed to exist.

On Saturday, June 18, 2016, Jakub Berezanski [email protected]
wrote:

Most native installers create log files simply as
$Env:TEMP.log (and that is, for example, where I typically
look for the log file when I need to troubleshoot an installation). I
suggest we put that as a recommendation for package authors to follow.

As for packages using TEMP\chocolatey (or TEMP\chocolatey),
correct me if I'm wrong, but have those directories ever been actually
documented? I believe they have not and packages putting log files there
should be treated as relying on non-contractual implementation details. The
package validator (or verifier? the one that performs the actual
installation - I always confuse the two) could actually detect this and
flag it as package error.

To mitigate this situation, after performing the
installation/uninstallation of a package, Chocolatey could scan the
TEMP\chocolatey directory and move all .log and .txt files it
finds to TEMP, with a warning.


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#813 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAD4Dg91MJFgSTWc2mmsL3qHqHayXk67ks5qNDmPgaJpZM4I4_tZ
.

Rob
"Be passionate in all you do"

http://codebetter.com/robreynolds
http://ferventcoder.com
https://twitter.com/ferventcoder

@jberezanski
Copy link

Good point about TEMP being restricted in some environments. In that case, substitute (cacheLocation ?? TEMP) for TEMP in my recommendation. But wouldn't packages using TEMP\chocolatey as log path fail in these environments anyway?

I agree that ensuring the folder exists is a simple and quick way to fix the issue without putting any burden on package maintainers. 👍

There is one ramification of leaving the logs in TEMP\chocolatey - the logs will be deleted in cases specified in 353c2da. This might be undesirable for some folks. In particular, this makes examination of the log generated during a successful uninstall impossible, as it will be immediately deleted. That is why I stand by my suggestion to move log files out of the package cache directory - perhaps to a new directory, (cacheLocation ?? TEMP)\chocolatey\logs?

@ferventcoder
Copy link
Member Author

@marcinbojko Right now that would be broken because $env:TEMP in package
installs evaluates to the equivalent of $env:TEMP\chocolatey (actual TEMP +
chocolatey), then the addition of your chocolatey for
"$env:TEMP\chocolatey\chocolatey".

Please consider this a gating bug that will be resolved this weekend - no
need to adjust your scripts.

On Saturday, June 18, 2016, marcinbojko [email protected] wrote:

For most of our popular packages, I've had to set $logfile to
"$env:TEMP\chocolatey$($packageName).MsiInstall.log"
Works as a workaround.


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#813 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAD4DqisWz-plKMMAN4V7xD0niYu6fyjks5qNDYugaJpZM4I4_tZ
.

Rob
"Be passionate in all you do"

http://codebetter.com/robreynolds
http://ferventcoder.com
https://twitter.com/ferventcoder

@ferventcoder
Copy link
Member Author

ferventcoder commented Jun 19, 2016

In particular, this makes examination of the log generated during a successful uninstall impossible, as it will be immediately deleted.

@jberezanski When it comes to uninstall logs, folks are free to put that file where ever they want. We don't make any suggestions about where that should go. So if they put it in the package folder cache and it deletes it, that's really more on them to figure out. Perhaps a misinterpretation of my earlier comments? We only go down to TEMP\chocolatey with the $env:TEMP variable.

The only other two times those log files would get deleted is just prior to forcing an install / upgrade, which those files would likely get overwritten anyway. In the case of the forced install, this is exactly the case. In the case of the forced upgrade, it's quite likely.

BTW, using --force should not be an argument normally passed to Chocolatey commands, it subverts some of the smarts that Chocolatey has because it interprets force as "the user knows what they are doing and want to override Chocolatey's normal behavior".

@marcinbojko
Copy link

@ferventcoder , I figured out that much, but wanted ensure at least few working packages for my colleagues.

@ferventcoder
Copy link
Member Author

0.9.10.2 is moving through the automated moderation process now.

ferventcoder added a commit that referenced this issue Jun 19, 2016
Make the default value that comes out of the template for logging
use a much shorter path.
ferventcoder added a commit that referenced this issue Jun 19, 2016
When using $env:TEMP in versions of Chocolatey less than 0.9.10.x, TEMP
did not include "chocolatey". However, due to GH-590 and a9519b5, this
is now causing issues when someone has setup an MSI logging file with
`$env:TEMP\chocolatey\packagename\packagename.msi.log`. The package TEMP
value evaluates to `$env:TEMP\chocolatey` (actual TEMP variable), which
results in
`$env:TEMP\chocolatey\chocolatey\packagename\packagename.msi.log`. This
breaks because that folder doesn't get created.

Instead look for double chocolatey folder in actual evaluation and
handle that to ensure compatibility with packages.
ferventcoder added a commit that referenced this issue Jun 19, 2016
* stable:
  (version) 0.9.10.2
  (doc) update CHANGELOG/nuspec
  (GH-758) Ensure log path exists
  (GH-813) Fix double chocolatey logging folder
  (GH-813) Shorten Template default log path
  (doc) update default options help messages
  (maint) Don't log creation of folder
  (maint) formatting / add message consistency
  (GH-814) Ensure any version of choco
  (GH-811) Skip resource / licensed assemblies
  (version) 0.9.10.1
  (doc) update CHANGELOG/nuspec
  (GH-810) Install of choco sets exit code
  (GH-812) Upgrade 7zip to 16.02 to address CVEs
  (doc) Note functions Calling Set-PowerShellExitCode
  (GH-810) Fix - Cannot bind parameter exitCode
ferventcoder added a commit that referenced this issue Sep 2, 2016
For GH-813 (ae2e857), the double `chocolatey\chocolatey` folder was
removed from `$silentArgs` and `$additionalInstallArgs`, but `$file`
was missed. These changes were originally necessary due to changes in
GH-590 (a9519b5). Ensure that $file is also set properly so it no
longer uses a double chocolatey folder.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants