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

chore: CI: build 64-bit platforms consistently with GMP #5144

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Kha
Copy link
Member

@Kha Kha commented Aug 23, 2024

@Kha Kha added the release-ci Enable all CI checks for a PR, like is done for releases label Aug 23, 2024
@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 26, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 390a9a63a24ca28306a60faa0381d4292d23af95 --onto 7845a05cf1094f24a5c4a51c32dd84bf4ff31a54. (2024-09-26 10:11:30)

@digama0
Copy link
Collaborator

digama0 commented Sep 26, 2024

While we're at it with the olean version bump, can we add a bit of data which explicitly indicates GMP-ness in the file? Also, it would be extremely useful to have the lean version string (i.e. v4.12.0-rc1) inside the file, with the field null or empty for custom, nightly, PR, fork toolchains. Right now the only version information available is the githash and this is useless for inequality queries.

@eric-wieser
Copy link
Contributor

Such information could be used in digama0/oleandump#4, instead of the user having to tell oleandump which platform the oleans they are inspecting were built on.

false;
#endif
bool:7 unused = 0;
// 41 bytes: build githash, padded with `\0` to the right
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the hex-encoded git hash rather than the raw 20-byte SHA, right?

Copy link
Collaborator

@digama0 digama0 Oct 8, 2024

Choose a reason for hiding this comment

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

Yes. (I had a version of .lgz which parsed this and re-encoded it as a 20-byte sha, but it had invertibility issues in the case where there is something weird in this field.)

@digama0
Copy link
Collaborator

digama0 commented Oct 8, 2024

Thanks for adding the GMP flag. What are your thoughts on including the version information? (IIRC @eric-wieser also had a request for olean v2 but I forget what it is and it might be covered by my two requests.) If encoded as (4u8, 13u8, 0u8, 1u8) for v4.13.0-rc1 RCs with rc = 255 meaning the stable, then it can be done as pure lexicographic order, but encoding as a string is also an option which leaves more room for encoding unusual versions at the risk of underspecifying how version ordering should apply on these versions.

@eric-wieser
Copy link
Contributor

Perhaps you're thinking of me talking about #2908? This would be a change to what ends up in the olean, but not one that breaks any downstream readers, so is orthogonal to any versioning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants