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

do not assume /tmp is a valid location for temporary files #3845

Closed
krader1961 opened this issue Feb 12, 2017 · 4 comments
Closed

do not assume /tmp is a valid location for temporary files #3845

krader1961 opened this issue Feb 12, 2017 · 4 comments

Comments

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Feb 12, 2017

We have places in our code that assume /tmp/ is a valid location for temporary files. That is an invalid assumption. Such as when running in Termux for Android devices. PR #3604 was opened to address this but the initial change had some issues and the author of change has not addressed the problems in the 2.5 months since the pull-request was created. So I'm opening this issue to remind us to fix this sooner rather than later.

The recommended sequence is using the $TMPDIR env var if it's defined, else the P_tmpdir macro from stdio.h if it is defined, else _PATH_TMP from paths.h. This should be implemented as a function that everyplace that needs to create a temporary file can call.

@nyeecola
Copy link

@nyeecola nyeecola commented Apr 6, 2017

I would like to try and fix this issue, but I'm new here so I have a few questions.

Should I make env_universal_t store a global variable for the /tmp/ path? Or just a separate function as you mentioned?
Also, I saw that in _PATH_TMP is defined in a header called <paths.h>. Is it safe to assume that every platform we support has that same header? If not, won't that break the build?

@krader1961
Copy link
Contributor Author

@krader1961 krader1961 commented Apr 7, 2017

@nyeecola, Thank you for taking an interest in improving fish. You should not create or rely on a universal variable for this issue. This PR comment documents the best practice for deriving a temporary file directory. What is needed is an API within fish that can be called to return a directory for temporary files.

@nyeecola
Copy link

@nyeecola nyeecola commented Apr 7, 2017

@krader1961 Ok, I'll do as you say.

But if you don't mind me asking, why is it not okay to create a global variable for this? You don't need to answer this if you don't want to. (or don't have the time)

Anyway thanks for the answer. I'll try to fix this as soon as I can. (Probably next week.)

@krader1961
Copy link
Contributor Author

@krader1961 krader1961 commented Apr 7, 2017

A universal variable is not the same thing as a global variable in fish. Creating a C++ variable with global scope might be okay. But since we also need a function that can be handed a path relative to the temp dir and which returns an absolute path prefixed with the correct temp directory it probably makes more sense to create another function that returns just the temp dir path name. Global variables should, in general, be avoided.

bmalehorn added a commit to bmalehorn/fish-shell that referenced this issue Feb 6, 2019
`/tmp` isn't present / writeable on every system. Instead of always
using `/tmp`, try to use standard environment variables and
configuration to find a temporary directory.

Adapted from fish-shell#3974, with updates based on those comments.

Closes fish-shell#3845.
bmalehorn added a commit to bmalehorn/fish-shell that referenced this issue Feb 6, 2019
`/tmp` isn't present / writeable on every system. Instead of always
using `/tmp`, try to use standard environment variables and
configuration to find a temporary directory.

Adapted from fish-shell#3974, with updates based on those comments.

Closes fish-shell#3845.
@faho faho removed this from the fish-future milestone Feb 20, 2019
@faho faho added this to the fish 3.1.0 milestone Feb 20, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants