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

Mark temporary directories as excluded from macOS Spotlight #8686

Closed
wants to merge 2 commits into from

Conversation

kornelski
Copy link
Contributor

@kornelski kornelski commented Sep 7, 2020

#8684

macOS doesn't support CACHEDIR.TAG, and Cargo doesn't use correct temporary dir locations for macOS. As a workaround, directories can be excluded from macOS Spotlight indexing by adding .noindex suffix.

I've added the suffix only to build and incremental. Ideally .fingerprint and deps should have it too, but deps appears in user-visible paths and .fingerprint seems to be an experimental public API, so I haven't changed these two.


This may be better served by #8063

@rust-highfive
Copy link

r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2020
@kornelski kornelski marked this pull request as draft September 7, 2020 12:46
@alexcrichton
Copy link
Member

We've historically had friction trying to rename build directories due to how baked in the current structure is to other external tools, but is there perhaps a way to exclude items from spotlight with file attributes rather than by filename? (something like what we do for time machine for example)

@joshtriplett
Copy link
Member

joshtriplett commented Sep 9, 2020

Yeah, having to use the name for this seems painful.

Might be preferable to put it in one of the standard macOS directories that get excluded, or putting it somewhere in the .cargo directory (since those don't get indexed). We've already talked about using .cargo for that for other reasons (shared caches and similar).

@joshtriplett
Copy link
Member

Another possibility: if there's any concern that something might be looking in these directories, we could use build.noindex and incremental.noindex and similar and then symlink build -> build.noindex and incremental -> incremental.noindex.

@kornelski
Copy link
Contributor Author

@alexcrichton AFAIK there's no filesystem attribute for this. .noindex is the only still-working method that I've found, and it's pretty obscure. It looks like Apple really doesn't want to support temporary files anywhere other than the dedicated system temp/cache directories.

@kornelski
Copy link
Contributor Author

Symlinks sound like an easy way out. How about taking it further and symlinking to ~/Library/Caches/Cargo/<hash of the project directory>?

@tesuji
Copy link
Contributor

tesuji commented Sep 10, 2020

This doesn't affect old installation on macOS, right ?

@alexcrichton
Copy link
Member

It's true that a possible alternative is to place everything in the system default location, but Cargo isn't necessarily the best prepared for that. One thing I think we'd need to handle is cargo clean should be sure to have basically the same behavior that it has today. Another worry we've had in the past is that we hard-link a lot of files in Cargo between intermediate locations and their final location. By moving across things like your home directory we run the risk of more frequently spanning volumes, causing hard links to fail. This is mostly only an issue for Linux, though, since I think on macOS multi-volume home directories is much rarer.

Overall this is unfortunately a tricky problem in that renaming build directories is ripe for breakage. When we discussed this the idea of symlinks seemed like they could possibly fix the issue, though, so I think it might be worthwhile to explore what a solution might look like where caches are placed in more system-friendly directories (e.g. ~/Library/Caches/Cargo on macOS, and the equivalents on Windows and Linux) and symlinked into the same locations we've used historically.

@alexcrichton
Copy link
Member

Or well there's also always the gotcha that symlinks are difficult/different on Windows. I think we can get away with "directory junctions" regardless of user preferences, which may work for our purposes as well, but we'd have to investigate.

@kornelski
Copy link
Contributor Author

Thank you for the input. Here's the plan:

  • I'll make existing target folders keep everything as-is (real directories) to improve back compat and ease migration.
  • I'll limit this change to cfg(unix) to avoid the issues of directory junctions. This will make it a platform-dependent behaviour, but Windows is a differently-behaving platform.
  • New target/ directories will have symlinks to a subdirectory in system's temp if it's on the same device. Otherwise keeps existing behavior.
  • To keep this change small, the the subdirectory in temp will be unique per workspace to avoid adding new form of sharing of build artifacts (these dirs could be unified later)
  • I'll verify that cargo clean can clean inside the symlinked folders.

@joshtriplett
Copy link
Member

joshtriplett commented Sep 15, 2020 via email

@alexcrichton
Copy link
Member

That sounds about right I believe yeah. A possible further simplification, though, is that target/debug itself could point to a system-stored location. That way we don't have to worry about cross-device links and such (and we could opportunistically try it on Windows, handling errors and making a directory if it doesn't work). That way everything, not just some folders, would reside in the temporary directory location, but it should look the same to all existing users except that the top-level debug and release folders would actually reside somewhere else.

We'll still want to test this though since env::current_exe() may return a path which no longer resides in the project directory, which may break some projects. We can always tweak if necessary if that comes up though!

@kornelski
Copy link
Contributor Author

kornelski commented Sep 15, 2020

target/debug symlink would be a bit easier, but personally I think it's good to separate "build products" (cargo calls them targets) from "build intermediaries", so I think target/debug/$binary belongs where it is, since it's only a handful of files and they shouldn't suddenly disappear when the OS thinks it's time to reclaim disk space. The thousands of other temp files are the problem.

@alexcrichton
Copy link
Member

I think deps may fill up with a lot of unused stuff as well, but in any case this seems like it's ripe for experimentation to see what works best!

@bors
Copy link
Contributor

bors commented Sep 27, 2020

☔ The latest upstream changes (presumably #8739) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@@ -170,6 +172,7 @@ impl Layout {
fingerprint: dest.join(".fingerprint"),
examples: dest.join("examples"),
doc: root.join("doc"),
temp_root: Self::temp_root_path(&root),
Copy link

@soc soc Oct 12, 2020

Choose a reason for hiding this comment

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

The naming is a bit confusing, because cache and temp are usually different things and people people have different conceptions about them.

if path.exists() {
return Some(path.join("cargo"));
}
None
Copy link

@soc soc Oct 12, 2020

Choose a reason for hiding this comment

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

Not sure I understand this logic – is there any reason why it ignores the "important" part of XDG and instead does something completely different altogether in the "env not set" case?

More generally, reimplementing the directory-selection logic seems to be a straight-forward route to bikeshed city (as demonstrated in the RFC).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What "important" part do you mean?

I am afraid that touching XDG is going to attract bikeshed fiesta. I'm tempted to remove it for that reason, and go straight for /var/tmp instead.

Copy link

@soc soc Oct 12, 2020

Choose a reason for hiding this comment

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

What "important" part do you mean?

XDG defines default directories if the env var is not set. The env var is probably not set in 99% of the cases, because people are fine with the defaults.

Only "being XDG" when the env var is set ... if I wanted to troll people, that's how I would do it. ;-)

I am afraid that touching XDG is going to attract bikeshed fiesta.

Did you consider simply using a library?

I'm tempted to remove it for that reason, and go straight for /var/tmp instead.

That's a cheap way out for this special case, but I'm not sure you want the next person following this precedent when they try to fix the issue for config/data files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've considered using a library, but it this area is full of abandoned projects and half-abandoned forks, so I didn't see any suitable crate.

I think the best option would be to use a cargo-specific variable/setting for customizing this dir.

Copy link

Choose a reason for hiding this comment

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

I've considered using a library, but it this area is full of abandoned projects and half-abandoned forks, so I didn't see any suitable crate.

I think no crate can help you if your intention is to invent your own non-standard scheme anyway.

I think the best option would be to use a cargo-specific variable/setting for customizing this dir.

That's of course technically possible, but is does nothing to resolve rust-lang/rfcs#1615 which is about sane defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think no crate can help you if your intention is to invent your own non-standard scheme anyway.

Jabs like that are unhelpful. Please don't do that.

Copy link

Choose a reason for hiding this comment

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

Have a nice day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants