Skip to content

Conversation

@Rob23oba
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the release-ci Enable all CI checks for a PR, like is done for releases label Sep 30, 2025
@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 Sep 30, 2025
@leanprover-community-bot
Copy link
Collaborator

Mathlib CI status (docs):

  • ❗ Mathlib CI can not be attempted yet, as the nightly-testing-2025-09-30 tag does not exist there yet. We will retry when you push more commits. If you rebase your branch onto nightly-with-mathlib, Mathlib CI should run now. You can force Mathlib CI using the force-mathlib-ci label. (2025-09-30 12:20:30)

@leanprover-bot
Copy link
Collaborator

Reference manual CI status:

  • ❗ Reference manual CI can not be attempted yet, as the nightly-testing-2025-09-30 tag does not exist there yet. We will retry when you push more commits. If you rebase your branch onto nightly-with-manual, reference manual CI should run now. You can force reference manual CI using the force-manual-ci label. (2025-09-30 12:20:31)

@Kha
Copy link
Member

Kha commented Oct 10, 2025

@Rob23oba I'm curious, what is the status here? I guess we would have to adjust lean-llvm to ship llvm-objcopy

@ydewit
Copy link
Contributor

ydewit commented Oct 14, 2025

@Rob23oba I am assuming this pr will also simplify lakefile.lean? E.g.:

  ...
  nativeFacets := fun shouldExport =>
    if shouldExport then
      #[Module.oExportFacet, `module.oShimExportFacet]
    else
      #[Module.oNoExportFacet, `module.oShimNoExportFacet]

@Rob23oba
Copy link
Contributor Author

I don't really know enough about lakefiles to be able to answer that question. But @Kha, to answer your question, I'm not sure I'm completely happy about how fast it is, it still takes a few seconds to go through all the files (process creation overhead?). Also, I'm not very familiar with the lake code base so it'd be nice if @tydeu could look over those changes.

@Kha
Copy link
Member

Kha commented Oct 15, 2025

I'm not sure I'm completely happy about how fast it is, it still takes a few seconds to go through all the files (process creation overhead?).

Well that's still something like an improvement of two orders of magnitude over the previous code, isn't it?

@tydeu
Copy link
Member

tydeu commented Nov 3, 2025

@Rob23oba The general idea here looks good to me. Did you mean for the code to be ready for review? After looking at this PR again today, I realized that is not clear to me from the discussion who is waiting on who.

@Rob23oba
Copy link
Contributor Author

Rob23oba commented Nov 3, 2025

Right, I guess I should add: before realizing that there was already a solution for this problem (llvm-objcopy) I wrote my own piece of code that could remove the section in question based on the specification at https://learn.microsoft.com/en-us/windows/win32/debug/pe-format. After realizing the existence of llvm-objcopy I then wrote this PR using it. But after I tested it locally I thought that it was actually still pretty slow (~30 files/sec) and that it might be faster to use my original attempt at writing it myself since it's literally just ~300 loc and would have less complexity than llvm-objcopy. So I tested using it instead for the stage0 build but then found that it wasn't much of a difference, so I decided at first not to put it in this PR. But later, I tested it again, this time modified to traverse the file tree itself. That got, well, mixed results. One time it felt like it took roughly as long as through the stage0 make file but another time it took just around 2 seconds... not sure what exactly was going on each time. So, in general, I'm a bit inconclusive if we should keep it as-is (with llvm-objcopy) or if we should instead use the piece of C code I wrote.

@tydeu
Copy link
Member

tydeu commented Nov 3, 2025

@Rob23oba I would start with llvm-objcopy in this PR (since that already provides a significant speedup) and then potentially revamp it to more custom code (in a later PR) if that is shown to be a significant performance improvement. There is also a separate benefit to using a tool vendor by a third party rather than code we would need to maintain in the future.

@Rob23oba
Copy link
Contributor Author

Rob23oba commented Nov 4, 2025

Alright then, I guess I'd be ready for a review then

@Rob23oba Rob23oba marked this pull request as ready for review November 4, 2025 07:05
@Rob23oba Rob23oba requested a review from tydeu as a code owner November 4, 2025 07:05
@Kha Kha requested review from Kha and hargoniX November 11, 2025 13:17
@Kha
Copy link
Member

Kha commented Nov 11, 2025

I guess we would have to adjust lean-llvm to ship llvm-objcopy

This part is still outstanding, no? WIthout it, I assume this would have minimal impact for the average user

@leanprover-bot leanprover-bot added the P-medium We may work on this issue if we find the time label Nov 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

6 participants