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

feat: use Lake.withLockFile in cache #6611

Closed
wants to merge 5 commits into from
Closed

feat: use Lake.withLockFile in cache #6611

wants to merge 5 commits into from

Conversation

semorrison
Copy link
Contributor

@semorrison semorrison commented Aug 16, 2023


Open in Gitpod

@semorrison semorrison added blocked-by-other-PR This PR depends on another PR to Mathlib blocked-by-core-PR This PR depends on a PR to Core/Std labels Aug 16, 2023
@semorrison
Copy link
Contributor Author

semorrison commented Aug 16, 2023

It might be a while before we can merge this! (Edit: mistakenly thought we needed a Lean bump first.)

@semorrison
Copy link
Contributor Author

@tydeu, could you advise how an end-user should specify the path to the lockfile, rather than hard-coding as I've done here?

@tydeu
Copy link
Collaborator

tydeu commented Aug 16, 2023

@semorrison Why does this need a Lean bump? lake.lock landed before the current 08-15 nightly used by mathlib?

@tydeu
Copy link
Collaborator

tydeu commented Aug 16, 2023

@semorrison As to your question:

could you advise how an end-user should specify the path to the lockfile, rather than hard-coding as I've done here?

The filename (lake.lock) is available as Lake.lockFileName. To get the full path, you can load he configuration and using it to determine the location of the build directory and lock file. However, that is expensive and probably not worth it here. Hard coding it seems fine, especially as the build directory is already hard coded in cache elsewhere (e.g., LIBDIR). Eventually, a solution to leanprover/lean4#2753 may help solve this.

@semorrison
Copy link
Contributor Author

@semorrison Why does this need a Lean bump? lake.lock landed before the current 08-15 nightly used by mathlib?

Absolutely right, I must not have branched off the latest master on my first attempt at this. I will force push this now.

@semorrison semorrison added awaiting-review awaiting-CI and removed blocked-by-other-PR This PR depends on another PR to Mathlib blocked-by-core-PR This PR depends on a PR to Core/Std labels Aug 16, 2023
@alreadydone
Copy link
Contributor

alreadydone commented Aug 17, 2023

(Edit: Oh nevermind, I see it's reported on Zuliip.)

My PR fails CI run and the only error (I think) concerns lake.lock. Do you know what's happening? Thanks.

[3800/3804] Building Mathlib.NumberTheory.ZetaFunction
[3802/3804] Building Mathlib.AlgebraicGeometry.Morphisms.FiniteType
[3803/3804] Building Mathlib
error: no such file or directory (error code: 2)
  file: ./build/lake.lock
Error: The process '/usr/bin/bash' failed with exit code 1
Remove matcher: gcc

@tydeu
Copy link
Collaborator

tydeu commented Aug 17, 2023

@semorrison One issue that just occurred to me with this PR is that the cache lock will conflict with Lake's in #6603. Thus, there needs to be a --no-lock flag in cache (or something similar) to disable locking when called from within the build process.

@tydeu tydeu added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review labels Aug 17, 2023
@semorrison semorrison added awaiting-review and removed awaiting-author A reviewer has asked the author a question or requested changes labels Aug 21, 2023
@semorrison semorrison requested a review from tydeu August 21, 2023 02:02
@semorrison
Copy link
Contributor Author

I've implemented --no-lock.

Really we should be using the lean4-cli library here, but as we want to replace cache anyway I don't think it's worth doing, so I've just added to step to the argument mangling.

Copy link
Collaborator

@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.

LGTM.

Cache/Main.lean Outdated Show resolved Hide resolved
Co-authored-by: Mac Malone <mac@lean-fro.org>
@semorrison
Copy link
Contributor Author

Marking as WIP, as we have temporarily reverted the lockfile.

@semorrison semorrison added WIP Work in progress and removed awaiting-review labels Aug 23, 2023
@bors bors bot changed the base branch from master to ScottCarnahan/BinomialRing2 September 17, 2023 03:26
@semorrison semorrison changed the base branch from ScottCarnahan/BinomialRing2 to master September 17, 2023 12:12
@semorrison semorrison closed this Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants