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

fix the location for yarn-error.log and add a test case #2870

Closed

Conversation

clanghout
Copy link

Summary
In order to change the folder where yarn-error.log is placed, we changed the path from config.cwd to config.cacheFolder. In the process of testing we also discovered a bug with specifying a cache folder from the command line.
The test executes a command that will error a yarn-error.log and verifies that this log is created in the specified cache folder.
Fixes #2719

Test plan
Run yarn test

or by hand: run the command yarn tag rm non-existing-pkg non-existing-tag and press enter, then verify that the reporter put the error log in the cache folder.

@TimvdLippe
Copy link
Contributor

It seems that CircleCI is timing out on __tests__/commands/install/integration.js (357.669s) and the same for AppVeyor: __tests__\commands\install\integration.js (275.532s)

On Travis all succeeded, but the 1 job on OS X is failing. That might be a mac related issue.

@TimvdLippe
Copy link
Contributor

On all CI's, the integration test is timing out. However, our own test case is not the culprit any longer. The last commit fixes the path issue on OS X.

Copy link
Member

@bestander bestander left a comment

Choose a reason for hiding this comment

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

Yarn puts the error log into CWD the same way npm does.
Why would we want it in cache folder?

@@ -362,7 +362,7 @@ config.init({
binLinks: commander.binLinks,
modulesFolder: commander.modulesFolder,
globalFolder: commander.globalFolder,
cacheRootFolder: commander.cacheFolder,
cacheFolder: commander.cacheFolder,
Copy link
Member

Choose a reason for hiding this comment

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

This is done on purpose afaik.
@arcanis we probably need to add a comment here.

Copy link
Author

Choose a reason for hiding this comment

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

This probably does need a change to either _cacheRootFolder or cacheFolder since cacheRootFolder does not exist in the configOptions export type. The reason for this change was the cache folder location not changing when setting it with the --cache-folder command (see #2881).

Copy link
Member

@arcanis arcanis Mar 10, 2017

Choose a reason for hiding this comment

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

Yep the specific line where this is used is here: https://github.com/yarnpkg/yarn/blob/master/src/config.js#L241

In order to prevent BC break we decided to keep using cacheFolder in the public interface instead of cacheRootFolder, but it seems I forgot one. Good catch @clanghout!

@clanghout
Copy link
Author

clanghout commented Mar 10, 2017

Since the release of npm v4.2.0 the error logs are placed in the cache folder instead of the CWD:

This debug log would get cleared out as soon as you ran npm again. This was a bit annoying because 1) you would get a random file in your git status that you might accidentally commit, and 2) if you hit a hard-to-reproduce bug and instinctually tried again, you would no longer have access to the repro npm-debug.log.

The cache will now hold a (configurable) number of npm-debug.log files, which you can access in the future.

source: npm release v4.2.0.
What differs however (and I have not implemented) is saving multiple log files inside the cache folder.

@TimvdLippe
Copy link
Contributor

Using process.exec does not generate any coverage. We might be able to directly require cli/index.js and then pass the arguments.

@bestander
Copy link
Member

Still it seems weird to have the error in the cache folder.
I understand the risk is people may commit it accidentally.

@bestander
Copy link
Member

I guess doing same as npm with <cache>/_logs and adding timestamp to the filename could be an option.
And also Yarn should log the error file location if it does not.

@clanghout
Copy link
Author

Different behavior of the cache folder is implemented in #3367 and the found bug is merged in #3033

@clanghout clanghout closed this May 11, 2017
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