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

refactor: .lake directory for Lake outputs #2749

Merged
merged 5 commits into from
Nov 14, 2023
Merged

Conversation

tydeu
Copy link
Member

@tydeu tydeu commented Oct 25, 2023

Store the build directory, build archives, package directory, and compiled configuration inside a dedicated directory for Lake outputs (i.e., .lake). There is also now a more rigorous check on lakefile.olean compatibility. (edit: split out into #2842)

Closes #2713.

@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 Oct 25, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 25, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Oct 25, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Oct 25, 2023

@tydeu
Copy link
Member Author

tydeu commented Oct 25, 2023

@Kha I was going to ask you if the IO.FS additions looked good since they were outside my module ownership. Does your 🎉 imply you are satisfied with them?

@tydeu
Copy link
Member Author

tydeu commented Oct 25, 2023

@semorrison A note for you when you can take a look: The mathlib CI is currently breaking for reasons other than this PR and I think this PR will likely need a proper CI run on mathlib since it may break some assumptions there about the location of the build directory.

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 25, 2023
@kim-em
Copy link
Collaborator

kim-em commented Oct 26, 2023

Options for a successful PR run:

  • rebase this PR onto d126c09 (the commit underlying nightly-2023-10-25), which should give an immediate run.
  • wait until nightly-2023-10-26 lands and I have fixed Std+Mathlib for it (approximately +10 hours from now), and then push an empty commit (or anything) to this branch.

@kim-em kim-em removed the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Oct 26, 2023
@Kha
Copy link
Member

Kha commented Oct 26, 2023

@Kha I was going to ask you if the IO.FS additions looked good since they were outside my module ownership. Does your 🎉 imply you are satisfied with them?

I see they are being used in the lockfile code but I can't tell what's going on for lack of comments. What is the difference to readFile/writeFile?

@tydeu
Copy link
Member Author

tydeu commented Oct 26, 2023

@Kha

What is the difference to readFile/writeFile?

readFile/writeFile would allow two racing lake processes to load a bad olean (e.g., one writes an updating lock and then another loads the old olean before the other writes the new one, or one writes a new olean and another loads it before the other writes the new lock). The new primitives allow us to open a handle, read the old hash, and not release it until we write the new one. I will add a comment to this effect in the code.

@Kha
Copy link
Member

Kha commented Oct 26, 2023

The new primitives allow us to open a handle, read the old hash, and not release it until we write the new one.

Mmh, if I understand you correctly and you believe that open/fdopen locks the file against concurrent access from other processes, that is simply untrue I'm afraid. That is, it is still racy.

@tydeu
Copy link
Member Author

tydeu commented Oct 26, 2023

@Kha

that is simply untrue I'm afraid. That is, it is still racy.

Oops, yeah, it appears I confused the exclusion offered by e.g. O_EXCL in writeNew with general file handles. 🤦‍♂️ I guess I actually also needed lock/unlock file handle bindings for flock, LockFileEx, and UnlockFileEx.

@tydeu tydeu force-pushed the lake-dir branch 2 times, most recently from ab82bca to 122f856 Compare October 27, 2023 07:07
@Kha
Copy link
Member

Kha commented Oct 27, 2023

I guess I actually also needed lock/unlock file handle bindings for flock, LockFileEx, and UnlockFileEx.

Phew, I'm impressed you made that work in quick time but this looks like it will take some time to review. Why not leave worrying about races to #2766 as mentioned there? On a positive note, this looks like the right functions to implement a general build lock that avoids the issues of the first try.

@Kha
Copy link
Member

Kha commented Oct 27, 2023

@joehendrix Speaking in general, I think it would be great if you could review additions to Init.System.IO and similar modules from a standard library perspective - they may as well be in std4 if it wasn't for bootstrapping. But for this one, let's first wait which functions we actually need in the end.

@tydeu
Copy link
Member Author

tydeu commented Oct 27, 2023

Anyway, this should be done. The last platform addition was some thing I was supposed to add after a discussion with Mario, but it slipped my mind until now. The lakefile.olean should now be properly lock-guarded to avoid race conditions and store enough information to be invalidated by Lake on any relevant change.

@tydeu tydeu added the awaiting-review Waiting for someone to review the PR label Oct 27, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 27, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Oct 27, 2023
@tydeu tydeu force-pushed the lake-dir branch 2 times, most recently from 7c15519 to 2092e25 Compare October 27, 2023 18:22
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Oct 27, 2023
@tydeu
Copy link
Member Author

tydeu commented Nov 7, 2023

I split the lakefile.olean improvement out into a separate PR that builds on this one (#2842). Hopefully, this will be cleaner.

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 7, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 8, 2023
@Kha
Copy link
Member

Kha commented Nov 8, 2023

I'll remove my review request, unless there's something specific you want me to look at

@tydeu tydeu changed the title refactor: _lake directory for Lake outputs refactor: .lake directory for Lake outputs Nov 8, 2023
@tydeu
Copy link
Member Author

tydeu commented Nov 8, 2023

Following the Zulip poll, I have decided to follow the majority and make the default Lake directory .lake. Furthermore, as this is now a pure-Lake PR, I will likely merge soon after the mathlib incompatibility is resolved, unless there are any objections.

/cc @semorrison, @Kha, @joehendrix: Since you all commented, it seems proper to notify all of you before I merge in case there is anything else you wish to add.

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 8, 2023
@Kha Kha removed their request for review November 8, 2023 20:27
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Nov 9, 2023
@tydeu tydeu removed the awaiting-mathlib We should not merge this until we have a successful Mathlib build label Nov 9, 2023
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 13, 2023
@tydeu tydeu merged commit 6d34920 into leanprover:master Nov 14, 2023
kim-em added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 15, 2023
@tydeu tydeu deleted the lake-dir branch November 16, 2023 00:31
github-merge-queue bot pushed a commit that referenced this pull request Nov 27, 2023
This is an additional safety net on top of #2749: it protects users that
circumvent the build system (e.g. with `lake env`) as well as obviates
the need for TOCTOU-like race condition checks in the build system.

The check is activated by `CHECK_OLEAN_VERSION=ON`, which now defaults
to `OFF` as the sensible default for local development. When activated,
`USE_GITHASH=ON` is also force-enabled for stage 0 in order to make sure
that stage 1 can load its own core library.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review Waiting for someone to review the PR builds-mathlib CI has verified that Mathlib builds against this PR 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.

lake upload fails with tar: file changed as we read it
5 participants