Closed bluelovers closed 4 years ago
@bluelovers this has been documented as one of the breaking changes in version 0.2.0 and was introduced as a precaution so that one cannot inject arbitrary directories into tmp have it remove these directories recursively, say /etc or something like that.
Is yarn-cache/tmp your default tmp directory? Or are you using multiple such temporary root for different purposes?
An option would be to pass in the system's TEMP as an environment variable, that way you could point your application to TEMP=T:\cache\yarn-cache\tmp and it will work. Otherwise, set up TEMP=T:\ and create some symlinks in there, e.g.
T:\yarn-cache-tmp -> T:\cache\yarn-cache\tmp
and use dir = yarn-cache-tmp, which should work as expected.
Alternatively you can omit the symlink and just use T:\cache\yarn-cache\tmp with TEMP = T:\
.
@silkentrance, our application doesn't use system temporary directory for some reasons. One of them is that this directory is cleaned and also used by some legacy software which fails working with system temporary directory due to permission reasons (some files in /tmp
can't be accessed and removed by regular user).
Symlinking temporary directory and passing its path as an environment variable are also not good options. Paths to temporary directories of our application are set in a separate configuration file and making additional efforts by linking directories and setting up environment variables don't allow our app to work out-of-box.
Wouldn't it be better to add an option that sets an alternative root temporary directory (or multiple root temporary directories) programatically? I don't see any graceful ways to make this version of tmp
useful in our case.
Another (and most important) reason for us is that we're using beegfs cluster FS where our software is processing data distributed on many computers. Having temporary directories in this file system for data processing is mandatory and doesn't suit locking temporary directory to OS' one.
Please, consider adding an option to remove these restrictions. We know what we are doing by removing such restrictions, and for sure, it will not affect important system directories such as /etc
because our software is not working under the root
.
Providing an option to explicitly skip this behavior sounds like a good middleground to me.
@raszi this is a sound proposal.
@ezze @bluelovers A temporary workaround for this, while the new release is under way, is to replace os.tmpdir() with a custom function like so (we do this in our tests and it works just fine):
const os = require('os');
os.tmpdir = function () { return 'T:\cache\yarn-cache\tmp'; };
Remember to cast os to any when using Typescript, e.g.
import * as os from 'os';
(os as any).tmpdir = function () { return 'T:\cache\yarn-cache\tmp'; };
~In the future then, tmp will provide an additional method that will let you replace the standard os tmpdir, i.e.~
const tmp = require('tmp');
tmp.setTmpDirProvider(function () {
return 'T:\cache\yarn-cache\tmp';
});
~I will explicitly model this as a provider function so that the app can easily switch between temporary roots.~
~Also, setting the tmp dir provider to null will revert the system back to os.tmpDir.~
This was a bad idea and will bring up unwanted side effects.
@raszi @ezze @bluelovers Alternatively, of course, one could set the process environment variable, which gets evaluated by os.tmpDir(), e.g.
by using https://itnext.io/how-to-use-environment-variables-in-node-js-cb2ef0e9574a?gi=c94b015c8288
or by setting it directly
process.env['TEMP'] = 'T:\cache\yarn-cache\tmp';
As always, there are multiple ways to achieve this without having to change the tmp API.
Let me know which approach would suit you best.
dotenv
is portable and requires no change to tmp.tmp.setTmpDirProvider(() -> void)
and make the change to tmpI don't think that any of those suggestion would work. Think about another projects depending on the environment variables or the os.tmpdir
. This should not be a global setting but per tmp request.
@silkentrance, I agree with @raszi. tmp
can also be used implicitly as a dependency of other dependencies of the project. This could lead to an unexpected behavior if we will alter this setting globally.
So we would then settle on an extra option that lets the user override the tmpdir location, e.g.
tmp.tmpNameSync({tmpdir: 'T:\cache\yarn-cache\tmp'});
tmp.dirSync({tmpdir: 'T:\cache\yarn-cache\tmp', 'dir': 'very-important-do-not-lose', unsafeCleanup: false});
and so on. This will then require the user to always pass in the tmpdir option.
I still do not see a problem in using the process environment for setting it globally. In other projects we always do it this way and it never caused any unwanted side effects.
Unless, of course, you have software that uses a hard coded path to some /tmp
location. I guess, this is where the software must actually fail, as it is bad practice.
I see the point about switching the tmp dir provider during runtime, though, as this will fail tmp if the user used a temporary directory created under a different path and then switches the tmp dir provider to point to a different root location. So this will definitely bring up unwanted side effects.
I also see the point of dynamically changing the process environment variable that points to the temporary storage, similar unwanted side effects can be had here as well.
Yes, that is the proposal I had in mind, optionally override the tmpdir location.
I still do not see a problem in using the process environment for setting it globally. In other projects we always do it this way and it never caused any unwanted side effects.
@silkentrance The problem comes when you use some dependency that relies on the same version of tmp
used in your project. Then you will get temporary files and directories from this nested dependency in your globally set temporary directory that is not what you probably want.
The best option to my taste is to bypass root tmp directory checks at all in order to avoid duplicated parts in dir
and tmpdir
proposed above.
I just found out that changes to the options will leak out. Also changing that.
@ezze It is a security feature that is required. Not all users are power users and some might wreck their systems when allowing that.
@silkentrance, ok, it's just my proposal. Changing root temporary directory for single tmp function call is also fine for us.
PR is here: #252
Operating System
NodeJS Version
Tmp Version
0.2.0
Expected Behavior
should allow any dir
Experienced Behavior
TBD:What did actually happen?