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: per-package server options #2858

Merged
merged 18 commits into from
Nov 26, 2023

Conversation

mhuisi
Copy link
Contributor

@mhuisi mhuisi commented Nov 10, 2023

This PR adds per-package server options to resolve #2455. It is based on the previous work in #2456, but takes a different approach: options are loaded for the specific file in the file worker when print-paths is called, instead of loading them in the watchdog with a separate Lake command. This change addresses review comments made in #2456.

In doing so, it introduces two new Lake config fields:

  • leanOptions: -D flag options that are passed to both the language server and lean when building.
  • moreServerOptions: -D flag options that are passed to the language server.

Since print-paths must also accept a file path to compute the options for that file, this PR is changing the API for print-paths. As there have been numerous complaints about the name print-paths, I also decided to change it to setup-file in this PR, since it would break compatibility with the old Lake API anyways.

This PR deprecates the Lakefile field moreServerArgs in favor of moreGlobalServerArgs, as suggested in the review for #2456.

Updating your Lakefile

A Lakefile with the following package declaration:

def moreServerArgs := #[
  "-Dpp.unicode.fun=true"
]
def moreLeanArgs := moreServerArgs

package SomePackage where
  moreServerArgs := moreServerArgs
  moreLeanArgs := moreLeanArgs

... can be updated to the following package declaration to use per-package options:

package SomePackage where
  leanOptions := ⟨`pp.unicode.fun, true⟩

A package configuration that does not set moreLeanArgs can use moreServerOptions instead of leanOptions:

package SomePackage where
  moreServerOptions := ⟨`pp.unicode.fun, true⟩

Fixes #2455

@mhuisi mhuisi added Lake Lake related issue awaiting-review Waiting for someone to review the PR labels Nov 10, 2023
@mhuisi mhuisi requested a review from tydeu November 10, 2023 13:51
@mhuisi
Copy link
Contributor Author

mhuisi commented Nov 10, 2023

(cc @Kha to adjust the Nix stuff)

@digama0 digama0 mentioned this pull request Nov 10, 2023
1 task
@mhuisi
Copy link
Contributor Author

mhuisi commented Nov 10, 2023

The tests are red because of a bug in System.Uri.fileUriToPath?. I will investigate it later.

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.

LGTM!

src/lake/Lake/CLI/Serve.lean Outdated Show resolved Hide resolved
@mhuisi
Copy link
Contributor Author

mhuisi commented Nov 10, 2023

The tests are red because of a bug in System.Uri.fileUriToPath?. I will investigate it later.

It's worse: The tests used an incorrect URI format and it never caused problems before!

@mhuisi mhuisi added server Affects the Lean server code and removed awaiting-review Waiting for someone to review the PR labels Nov 10, 2023
@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 Nov 10, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Nov 10, 2023

  • ❗ Mathlib CI will not be attempted unless you rebase your PR onto the 'nightly' branch. (2023-11-10 21:16:20)
  • 🟡 Mathlib branch lean-pr-testing-2858 build this PR didn't complete normally. (2023-11-10 21:19:14) View Log
  • ❗ Mathlib CI can not be attempted yet, as the 'nightly-testing-2023-11-14' branch does not exist there yet. We will retry when you push more commits. It may be necessary to rebase onto 'nightly' tomorrow. (2023-11-14 11:56:18)
  • ❌ Mathlib branch lean-pr-testing-2858 built against this PR, but lean4checker failed. (2023-11-20 04:50:47) View Log
  • ❗ Mathlib CI can not be attempted yet, as the 'nightly-testing-2023-11-20' branch does not exist there yet. We will retry when you push more commits. It may be necessary to rebase onto 'nightly' tomorrow. (2023-11-21 10:45:35)
  • ✅ Mathlib branch lean-pr-testing-2858 has successfully built against this PR. (2023-11-23 15:38:49) View Log
  • ❗ Mathlib CI can not be attempted yet, as the 'nightly-testing-2023-11-25' branch does not exist there yet. We will retry when you push more commits. It may be necessary to rebase onto 'nightly' tomorrow. (2023-11-25 20:34:20)
  • 💥 Mathlib branch lean-pr-testing-2858 build failed against this PR. (2023-11-25 21:26:00) View Log
  • ✅ Mathlib branch lean-pr-testing-2858 has successfully built against this PR. (2023-11-26 10:31:09) View Log

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

tydeu commented Nov 11, 2023

I missed something! Since you renamed moreServerArgs, the math package template needs to be updated as well:
https://github.com/mhuisi/lean4/blob/aa0774467e9ce6ab5f911bd06674593967283bbd/src/lake/Lake/CLI/Init.lean#L98-L118

Also, for temporary backwards compatibility, it might be worth it for one version to leave the PackageConfig.moreServerArgs field and have moreGlobalServerArgs default to it. To deprecate a property like this one, I usually put a note on its deprecation in the docstring, remove the mention of it in the README, and potentially a warning issue during package config load if it is non-empty (for reference, see how I deprecated manifestFile).

@semorrison
Copy link
Collaborator

Note that the Mathlib bot reporting that PR didn't complete normally is actually a failure:

Run lake exe cache clean
error: ./lakefile.lean:26:2: error: 'moreServerArgs' is not a field of structure 'Lake.PackageConfig'
error: ./lakefile.lean: package configuration has errors
Error: Process completed with exit code 1.

If you're able to make changes to the lean-pr-testing-2858 branch of Mathlib directory to make it compatible with these chaanges, please go ahead. Ping me if you need assistance on the Mathlib side.

@semorrison semorrison added the awaiting-mathlib We should not merge this until we have a successful Mathlib build label Nov 14, 2023
@mhuisi mhuisi force-pushed the mhuisi/per-package-server-options branch from aa07744 to 9dac96d Compare November 14, 2023 09:41
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 14, 2023
@mhuisi mhuisi force-pushed the mhuisi/per-package-server-options branch from 72d28d1 to f33dbc9 Compare November 16, 2023 16:07
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.

Some minor suggestions that help the code of the new options fit the style of other package configuration options. Otherwise, LGTM!

src/lake/Lake/Config/Package.lean Outdated Show resolved Hide resolved
src/lake/Lake/Config/Package.lean Show resolved Hide resolved
src/lake/Lake/Config/LeanLib.lean Outdated Show resolved Hide resolved
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.

@mhuisi I think you lost the library's leanOptions when addressing my suggestions.

src/lake/Lake/Config/LeanLib.lean Outdated Show resolved Hide resolved
src/lake/Lake/Config/LeanLib.lean Outdated Show resolved Hide resolved
@mhuisi mhuisi force-pushed the mhuisi/per-package-server-options branch from 181ab9e to bad3e43 Compare November 23, 2023 09:27
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added builds-mathlib CI has verified that Mathlib builds against this PR and removed awaiting-mathlib We should not merge this until we have a successful Mathlib build breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan labels Nov 23, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan builds-mathlib CI has verified that Mathlib builds against this PR and removed builds-mathlib CI has verified that Mathlib builds against this PR breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan labels Nov 25, 2023
@mhuisi mhuisi added this pull request to the merge queue Nov 26, 2023
Merged via the queue into leanprover:master with commit 7ff7cf9 Nov 26, 2023
18 checks passed
@mhuisi mhuisi mentioned this pull request Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builds-mathlib CI has verified that Mathlib builds against this PR Lake Lake related issue server Affects the Lean server code 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: per-package server options
6 participants