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

Fix Nix build header: F* and krml versions empty #394

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

Conversation

cmovcc
Copy link

@cmovcc cmovcc commented Oct 25, 2023

Use fstar.exe --version instead of git rev-parse HEAD in $FSTAR_HOME directory. Indeed, when building the F*+KaRaMeL toolchain in a Nix environment, $FSTAR_HOME does not point to a git repository.
I foresee a few discussions.

  • This is perhaps a convoluted way to retrieve the git revision (List.nth (...) 5), I was unsure about the portability of grep usage, for example when using MacOS. As a consequence, we rely on the current output pattern of fstar.exe --version, should this be documented somewhere ? Does this choice seem reasonable?
  • Additionally, this adds a git_rev_length variable to control the length of F*/KaRaMeL revisions that are present in the KaRaMeL default header and raise it from 8 to 10. Perhaps this could/should be raised further, e.g. to 40, to be future-proof without any need to bump it?
  • Do you think any of this should be wrapped within an exception handler?

@msprotz
Copy link
Contributor

msprotz commented Oct 25, 2023

Additionally, this adds a git_rev_length variable to control the length of F*/KaRaMeL revisions that are present in the KaRaMeL default header and raise it from 8 to 10. Perhaps this could/should be raised further, e.g. to 40, to be future-proof without any need to bump it?

What problem are you trying to solve? Have you seen hash collisions?

This is perhaps a convoluted way to retrieve the git revision (List.nth (...) 5), I was unsure about the portability of grep usage, for example when using MacOS. As a consequence, we rely on the current output pattern of fstar.exe --version, should this be documented somewhere ? Does this choice seem reasonable?

@mtzguido @tahina-pro is this output expected to be stable?

Do you think any of this should be wrapped within an exception handler?

unless you can show me a situation where an exception will be raised, no

@cmovcc
Copy link
Author

cmovcc commented Oct 26, 2023

What problem are you trying to solve? Have you seen hash collisions?

No, I haven't, and it remains very unlikely, even the Linux kernel keeps revision of length 12 without any ambiguity issue, afaik. I think that the header purpose is to provide one with information about the toolchain that has been used to produce the corresponding file, possibly long after the extraction. As I don't think the header would be less readable/practical with full git revisions, why shouldn't it be changed to use full git revisions? By the way, krml -version already yields the full git revision.

@mtzguido @tahina-pro is this output expected to be stable?

Do you think any of this should be wrapped within an exception handler?

unless you can show me a situation where an exception will be raised, no

I guess it depends on the previous question (thanks for cc-ing).

@mtzguido
Copy link
Member

This is perhaps a convoluted way to retrieve the git revision (List.nth (...) 5), I was unsure about the portability of grep usage, for example when using MacOS. As a consequence, we rely on the current output pattern of fstar.exe --version, should this be documented somewhere ? Does this choice seem reasonable?

@mtzguido @tahina-pro is this output expected to be stable?

While I don't foresee any changes, I wouldn't want to close the door on adding/removing/reordering some lines in it. Could this code just try to read each line and find the one that starts with commit=?

@msprotz
Copy link
Contributor

msprotz commented Oct 26, 2023

Good suggestion, Guido -- thanks.

Regarding the commit hash: for the sake of keeping things readable, let's hardcode 12 as the length (what is good for Linux is good for us). Do not use a global variable, just leave 12 in the code, and let's not bikeshed on this -- this is entirely opinion-driven, unrelated to the goal of this PR, and does not solve any problem encountered in the wild.

@msprotz
Copy link
Contributor

msprotz commented Oct 26, 2023

@cmovcc: KString.starts_with is the helper you want here, something like

KList.filter_some (fun l -> if KString.starts_with l "commit=" then Some (String.sub ... l ...) else None) lines (sketch)

@cmovcc cmovcc force-pushed the areitz/fix-nix-build-header branch from 8b7f6df to f198096 Compare October 27, 2023 13:43
use fstar.exe --version instead of
git rev-parse HEAD in $FSTAR_HOME;
bump git revision length from 8 to 12
@cmovcc cmovcc force-pushed the areitz/fix-nix-build-header branch from f198096 to de6337b Compare October 27, 2023 13:50
@cmovcc cmovcc marked this pull request as draft October 27, 2023 14:38
@cmovcc cmovcc force-pushed the areitz/fix-nix-build-header branch from aff02d3 to cee62a0 Compare October 27, 2023 14:53
use Version.version instead of
git rev-parse HEAD in $KRML_HOME
@cmovcc cmovcc force-pushed the areitz/fix-nix-build-header branch from cee62a0 to 96f97c9 Compare October 27, 2023 15:22
@cmovcc cmovcc marked this pull request as ready for review October 27, 2023 15:35
@cmovcc cmovcc changed the title Fix Nix build header: F* version empty Fix Nix build header: F* and krml versions empty Oct 27, 2023
@cmovcc
Copy link
Author

cmovcc commented Oct 27, 2023

Thanks for the hint!

@R1kM R1kM enabled auto-merge March 3, 2024 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants