Skip to content

feat: create CACHEDIR.TAG file in .lake directory #5059

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

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

Conversation

henkkuli
Copy link

Create CACHEDIR.TAG file in .lake directory when it is first created. The file does not get recreated if removed if .lake directory is not removed, too. While technically against the spec, this is how Rust does it, too. Moreover, this simplifies the implementation a lot.

This change also introduces a test to check that the file gets cretead but not recreated. This test needs to change e.g. if we decide that CACHEDIR.TAG should always be recreated.


Closes #4998

Create CACHEDIR.TAG file in .lake directory when it is first created.
The file does not get recreated if removed if .lake directory is not
removed, too. While technically against the spec, this is how Rust does
it, too. Moreover, this simplifies the implementation a lot.

This change also introduces a test to check that the file gets cretead
but not recreated. This test needs to change e.g. if we decide that
CACHEDIR.TAG should always be recreated.
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Aug 15, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Aug 15, 2024

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 4d4d485c19925cf8c4122e553328ce320ff02439 --onto 8c96d213f3089ac2352ed95e79645f4cdbd70ebf. (2024-08-15 15:55:36)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 4d4d485c19925cf8c4122e553328ce320ff02439 --onto b486c6748b153bda628575635baa28aeeb38b8c5. (2024-08-20 09:31:18)

@henkkuli
Copy link
Author

Marking this as non-draft as I noticed that @tydeu has initially approved the idea on Zulip.

@henkkuli henkkuli marked this pull request as ready for review August 16, 2024 08:12
@henkkuli henkkuli requested a review from tydeu as a code owner August 16, 2024 08:12
@leanprover-bot leanprover-bot added the P-medium We may work on this issue if we find the time label Aug 16, 2024
@tydeu tydeu added the release-ci Enable all CI checks for a PR, like is done for releases label Aug 19, 2024
Copy link
Member

@tydeu tydeu left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me! I have one major suggested change, but, nonetheless, great job! I really like the detailed test. 😄

Comment on lines +271 to +272
-- Create CACHEDIR.TAG file only when .lake directory is first created
IO.FS.writeFile (cfg.lakeDir / "CACHEDIR.TAG") cachedirTagContent
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 not really what this this does. If a user deletes the CACHEDIR.TAG (because they wish the directory to be cached) and then, unrelatedly, delete the configuration trace (e.g., due to an incomplete configuration), this will regenerate the CACHEDIR.TAG (which would be quite surprising behavior).

Now that creating the lake directory is something worth doing independently, I suggest refactoring out the cfg.lakeDir creation here to the top of this function (outside all the locking). That is, just add something like this to the top and remove it from here:

if !(<- cfg.lakeDir.pathExists) then
  IO.FS.createDirAll cfg.lakeDir
  -- Create CACHEDIR.TAG file only when .lake directory is first created
  IO.FS.writeFile (cfg.lakeDir / "CACHEDIR.TAG") cachedirTagContent


$LAKE new hello
test ! -e hello/.lake
# Building craetes CACHEDIR.TAG
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Building craetes CACHEDIR.TAG
# Building creates CACHEDIR.TAG

Comment on lines +5 to +10
# Removing CACHEDIR.TAG without removing .lake directory and then rebuilding
# does not currently recreate CACHEDIR.TAG. This is an artifact of the current
# implementation and is technically against the spec:
#
# > The application should also regenerate the cache directory tag if it
# > disappears --
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 an odd spec requirement that does not seem desirable. A user may wish to remove the tag and cache the directory and Lake should not override that.

@nomeata nomeata added release-ci Enable all CI checks for a PR, like is done for releases and removed release-ci Enable all CI checks for a PR, like is done for releases labels Aug 20, 2024
@kim-em kim-em added the awaiting-author Waiting for PR author to address issues label Oct 25, 2024
@kim-em
Copy link
Collaborator

kim-em commented Oct 25, 2024

@henkkuli, would you be able to address @tydeu's review comments above? It would be nice to have this in.

@kim-em kim-em added the Lake Lake related issue label Oct 25, 2024
@github-actions github-actions bot added the stale label Nov 24, 2024
@github-actions github-actions bot removed the stale label Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author Waiting for PR author to address issues Lake Lake related issue P-medium We may work on this issue if we find the time release-ci Enable all CI checks for a PR, like is done for releases toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Exclude .lake directory from backups with CACHEDIR.TAG
6 participants