Skip to content

[Merged by Bors] - feat(Cache): extend argument-parsing to allow module names and file names#21822

Closed
joneugster wants to merge 52 commits intomasterfrom
eugster/cache_parse_args
Closed

[Merged by Bors] - feat(Cache): extend argument-parsing to allow module names and file names#21822
joneugster wants to merge 52 commits intomasterfrom
eugster/cache_parse_args

Conversation

@joneugster
Copy link
Copy Markdown
Contributor

@joneugster joneugster commented Feb 13, 2025

Accept both Mathlib.Init and Mathlib/Init.lean as arguments.


Note: I noticed a bug in walking directories which will not be present after #21834, so I put an error message "not implemented yet" here and will add the feature in the PR following #21834

Open in Gitpod

@joneugster joneugster added t-meta Tactics, attributes or user commands CI Modifies the continuous integration setup or other automation labels Feb 13, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 13, 2025

PR summary 4c5c94fd27

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference

Declarations diff

+ leanModulesFromSpec
+ parseArgs

You can run this locally as follows
## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>

The doc-module for script/declarations_diff.sh contains some details about this script.


No changes to technical debt.

You can run this locally as

./scripts/technical-debt-metrics.sh pr_summary
  • The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
  • The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

@joneugster joneugster changed the base branch from master to eugster/cache_remove_hardcoded_package_dirs February 13, 2025 12:19
@joneugster joneugster added the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Feb 13, 2025
@joneugster joneugster changed the base branch from eugster/cache_FileImports to master February 24, 2025 16:22
@mathlib4-dependent-issues-bot mathlib4-dependent-issues-bot removed the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Feb 27, 2025
@mathlib4-dependent-issues-bot
Copy link
Copy Markdown
Collaborator

@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Feb 27, 2025
@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Feb 28, 2025
Comment thread Cache/IO.lean Outdated
@joneugster joneugster added the awaiting-author A reviewer has asked the author a question or requested changes. label Feb 28, 2025
@joneugster joneugster added awaiting-author A reviewer has asked the author a question or requested changes. and removed awaiting-author A reviewer has asked the author a question or requested changes. labels Mar 5, 2025
@joneugster joneugster removed the awaiting-author A reviewer has asked the author a question or requested changes. label Mar 7, 2025
@joneugster joneugster changed the title feat(Cache): extend argument-parsing to allow module names, file names and directory names feat(Cache): extend argument-parsing to allow module names and file names Mar 7, 2025
Comment thread Cache/IO.lean

Note: An argument like `Archive` is treated as module, not a path.
-/
def leanModulesFromSpec (sp : SearchPath) (argₛ : String) :
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Arguably this is ill-defined; is Foo.Bar.lean a reference to ./Foo.Bar.lean or Foo/Bar/lean.lean?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(maybe the answer is to allow a leading ./ to disambiguate)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

currently it's a reference to ./Foo.Bar.lean especially since Mathlib.lean should be parsed that way. One add ./ but requiring it would not work for Mathlib.lean

Would it be desired to just try out both options with something like to following?

  if arg.extension == "lean" then
    let mut mods := #[]

    -- try if a file `Foo/Bar/lean.lean` exists
    let mod := argₛ.toName
    let sourceFile ← Lean.findLean sp mod
    if ← sourceFile.pathExists then
      mods := mods.append #[(mod, sourceFile)]

    -- try if a file `./Foo.Bar.lean` exists
    let mod₂ : Name := arg.withExtension "" |>.components.foldl .str .anonymous
    if (← arg.pathExists) then
      mods := mods.append #[(mod₂, arg)]
    return .ok mods

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Arguably this is ill-defined; is Foo.Bar.lean a reference to ./Foo.Bar.lean or Foo/Bar/lean.lean?

Alternatively, just assume that no one is going to do this. If they do, they can file a bug report, and we can tell them to stop being annoying. :-)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

At any rate we should make sure what we implement is consistent with whatever lean4#2756 decides.

Copy link
Copy Markdown
Contributor Author

@joneugster joneugster left a comment

Choose a reason for hiding this comment

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

Few comments to provide context

Comment thread Cache/IO.lean
Comment thread Cache/IO.lean
Comment thread Cache/Main.lean
Comment thread Cache/Main.lean
@kim-em
Copy link
Copy Markdown
Contributor

kim-em commented Mar 31, 2025

bors merge

@ghost ghost added the ready-to-merge This PR has been sent to bors. label Mar 31, 2025
mathlib-bors bot pushed a commit that referenced this pull request Mar 31, 2025
…ames (#21822)

Accept both `Mathlib.Init` and `Mathlib/Init.lean` as arguments. 



Co-authored-by: Eric Wieser <[email protected]>
@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Mar 31, 2025

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat(Cache): extend argument-parsing to allow module names and file names [Merged by Bors] - feat(Cache): extend argument-parsing to allow module names and file names Mar 31, 2025
@mathlib-bors mathlib-bors bot closed this Mar 31, 2025
@mathlib-bors mathlib-bors bot deleted the eugster/cache_parse_args branch March 31, 2025 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI Modifies the continuous integration setup or other automation ready-to-merge This PR has been sent to bors. t-meta Tactics, attributes or user commands

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants