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

Create temp files in temp directory #10366

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jasagredo
Copy link
Collaborator

@jasagredo jasagredo commented Sep 16, 2024

TL;DR

This change ensures all temporal files are created in the system temp directory which usually is in a short path. This helps with Windows not being capable of creating temp files in long directories, like the ones that result from Backpack.

Description

To be precise, purely temporary files (ghc intermediate files, rsps, etc) were already created in this system temporary directory, because the code called withTempFile always with the result from getTemporaryDirectory. I just made it so that the user doesn't even have the option to provide a different directory now.

And the other case is the writeFileAtomic function which used to create temporary files already in the destination directory and then rename them. If the destination directory was a long path, this would fail on Windows. This function now creates the temp file in the temp directory too.

Note that the "system temporary directory" is what getTemporaryDirectory returns which can be controlled via environment variables as $TMPDIR. See getTemporaryDirectory haddocks.

Reference

See how GetTempFileNameW specifies:

The string cannot be longer than MAX_PATH–14 characters or GetTempFileName
will fail.

And actually there is a TODO in Win32Utils.c in GHC:

https://gitlab.haskell.org/ghc/ghc/-/blob/3939a8bf93e27d8151aa1d92bf3ce10bbbc96a72/libraries/ghc-internal/cbits/Win32Utils.c#L259

Therefore Windows cannot create temporary files in long paths. To be precise, it can do so if using --io-manager=native but this is not yet the default and it seems reasonable to not make people face other complications that might arise from using that always, even more if it is as easy as creating the temp files somewhere else.


Closes #10191.

Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

@9999years, do the API changes here affect #10292?

@geekosaur
Copy link
Collaborator

(API changes: not backporting to 3.12.)

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@9999years
Copy link
Collaborator

@geekosaur I'm not sure, honestly. I can check later. They might make it harder to test stuff like in #10292 (comment)?

@jasagredo
Copy link
Collaborator Author

I think the only change is that one would have to look in the global tmp directory instead of in the destination directory.

@jasagredo jasagredo mentioned this pull request Sep 30, 2024
2 tasks
@mpickering
Copy link
Collaborator

I think the only change is that one would have to look in the global tmp directory instead of in the destination directory.

There might be anything in the global tmp directory, I think tests should run with a specific TMPDIR.

@jasagredo
Copy link
Collaborator Author

I find it reasonable that tests run with a dedicated TMPDIR, but is it ok that in general cabal uses the global temp dir instead of the destination path?

@geekosaur
Copy link
Collaborator

People expect to be able to control where cabal puts its files with $TMPDIR; you can search for TMPDIR in issues for examples.

@9999years
Copy link
Collaborator

People expect to be able to control where cabal puts its files with $TMPDIR; you can search for TMPDIR in issues for examples.

Oh yeah, I should've mentioned that we use this at Mercury earlier.

@jasagredo
Copy link
Collaborator Author

On Linux it will put temporary files in TEMPDIR. On Windows it will put temp files in TMP or TEMP. Linux users should be unaffected by temporary files (they were all created there already).

The only change is for writing files atomically via creating a temp file then renaming it to the proper filename. That used to happen in the destination directory, after this PR it will happen in TEMPDIR/TMP/TEMP.

@jasagredo jasagredo force-pushed the js/temp-files branch 2 times, most recently from de08ada to 91076d4 Compare September 30, 2024 21:18
@jasagredo
Copy link
Collaborator Author

jasagredo commented Sep 30, 2024

I added some more docs to writeFileAtomic, also reworked the test, and extensively reworded the description of the PR. I hope it is more clear now. @9999years @mpickering @geekosaur

@geekosaur
Copy link
Collaborator

It occurs to me that you should be able to put them in the current directory, because all tests are copied to a temp directory before running (see #9717).

This change ensures all temporal files are created in the system temp directory
which usually is in a short path. This helps with Windows not being capable of
creating temp files in long directories, like the ones that result from
Backpack.

See how GetTempFileNameW specifies:

> The string cannot be longer than `MAX_PATH–14` characters or `GetTempFileName`
will fail.

And actually there is a TODO in `Win32Utils.c` in GHC:

https://gitlab.haskell.org/ghc/ghc/-/blob/3939a8bf93e27d8151aa1d92bf3ce10bbbc96a72/libraries/ghc-internal/cbits/Win32Utils.c#L259

Closes haskell#10191.
@jasagredo jasagredo added the merge me Tell Mergify Bot to merge label Oct 1, 2024
@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-review merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure out why MAX_PATH is still a problem in Cabal
5 participants