Skip to content

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

Open
cmovcc wants to merge 5 commits intoFStarLang:masterfrom
cmovcc:areitz/fix-nix-build-header
Open

Fix Nix build header: F* and krml versions empty#394
cmovcc wants to merge 5 commits intoFStarLang:masterfrom
cmovcc:areitz/fix-nix-build-header

Conversation

@cmovcc
Copy link
Contributor

@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
Contributor 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
Contributor 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