Conversation
The all_fstar_files_in_dir function was unconditionally recursing into all subdirectories when --dep was given a directory argument. This changes it to use Find.expand_include_d, which only recurses into subdirectories listed in fstar.include files. Without fstar.include, only F* files in the immediate directory are found. With fstar.include, only the listed subdirectories are traversed (matching the existing behavior of the include path mechanism). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks a lot Lef! |
src/ml/FStarC_Filepath.ml
Outdated
| else | ||
| let path = normalize_file_path path in | ||
| let cwd = normalize_file_path (Sys.getcwd ()) in | ||
| let split s = List.filter (fun x -> x <> "") (BatString.nsplit s ~by:"/") in |
There was a problem hiding this comment.
It may be better to use Filename.dir_sep to be portable with Windows
(cf. https://ocaml.org/manual/5.3/api/Filename.html)
src/ml/FStarC_Filepath.ml
Outdated
| in | ||
| let (remaining_path, remaining_cwd) = skip_common path_parts cwd_parts in | ||
| let ups = List.map (fun _ -> "..") remaining_cwd in | ||
| String.concat "/" (ups @ remaining_path) |
src/ml/FStarC_Filepath.ml
Outdated
| | _ -> (pp, cp) | ||
| in | ||
| let (remaining_path, remaining_cwd) = skip_common path_parts cwd_parts in | ||
| let ups = List.map (fun _ -> "..") remaining_cwd in |
There was a problem hiding this comment.
OCaml has Filename.parent_dir_name, but since it is the same on all three of Windows, Linux and MacOS, we may not need to change this.
src/ml/FStarC_Filepath.ml
Outdated
| (* Sys.is_directory raises Sys_error if the path does not exist *) | ||
| let is_directory f = Sys.file_exists f && Sys.is_directory f | ||
|
|
||
| let make_relative_to_cwd (path:string) : string = |
There was a problem hiding this comment.
This could probably be implemented directly via batteries. It looks OK for *nix, not sure about Windows due to backslashes?
src/parser/FStarC.Parser.Dep.fst
Outdated
| let pr str = ignore <| FStarC.StringBuffer.add str sb in | ||
| let norm_path s = replace_chars (replace_chars s '\\' "/") ' ' "\\ " in | ||
| let norm_path s = | ||
| let s = Filepath.make_relative_to_cwd s in |
There was a problem hiding this comment.
So all paths will be relative now?
An explanation of why any absolute/relative distinction had to change would be useful. I feel like I'm still missing some context. I see this comment:
(** Expand any directories in the command line file list to their contained F* files.
When a module has both .fst and .fsti, only include the .fsti to avoid
"breaking abstraction" errors. *)
but I don't really understand it. Certainly it should not simply drop the fst since they are part of the dep analysis and should be verified+extracted etc.
There was a problem hiding this comment.
Yes,
- everparse uses
(filter-out src/lowparse/lowparse.%)on.depend(there are reasons for that which I understand but are too long to explain here @tahina-pro can elaborate in person) filter-outcannot take two wildcards (%src/lowparse/lowparse.%)
Those two reasons lead us to relative paths.
There was a problem hiding this comment.
Just so that I understand, is this logic in everparse new? Or was it working before? F* used to emit a mixture of relative and absolute paths according to what was passed for --depend. E.g.
$ ls d1/* d2/*
d1/X.fst d2/Y.fst
$ fstar.exe --dep full --include d1 --include $(pwd)/d2 > aThen ALL_FST_FILES has:
/home/guido/r/fstar/master/x/d2/Y.fst \
d1/X.fst
And it's the same for the targets:
d1/X.fst.checked: \
...
/home/guido/r/fstar/master/x/d2/Y.fst.checked: \
...
There was a problem hiding this comment.
Ok I understand that --include <relative or absolute> is the most general solution, I will do that.
There was a problem hiding this comment.
I'm not sure it's that important, just trying to understand. If using relative paths does not break (many) things in other projects then I'm OK with it. But I'm not sure why this changed at all.
There was a problem hiding this comment.
Just so that I understand, is this logic in everparse new? Or was it working before? F* used to emit a mixture of relative and absolute paths according to what was passed for --depend. E.g.
EverParse switched to a single Makefile for all dependencies in December 2024 at project-everest/everparse@19568d1 . It has been relying on relative --include paths for dependencies within EverParse, and absolute include paths for external dependencies (Karamel, Pulse, etc.) ever since.
5061426 to
04c562e
Compare
|
Alas, this change makes EverParse |
|
I don't get it, this is the previous behavior according to @mtzguido |
|
There is still a diff compared to F* master. This is a fraction of the diff I get for ALL_CHECKED_FILES in diff --git a/a b/b
index 9d92599..7baf62e 100644
--- a/a
+++ b/b
@@ -1,22 +1,25 @@
ALL_CHECKED_FILES= \
src/3d/prelude/EverParse3d.Prelude.fsti.checked \
- src/cddl/pulse/CDDL.Pulse.Serialize.Gen.MapGroup.ZeroOrMore.Aux2.Lemma11.fst.checked \
- src/cbor/spec/raw/CBOR.Spec.Raw.fst.checked \
+ /home/guido/r/everparse/main/src/cddl/pulse/CDDL.Pulse.Serialize.Gen.MapGroup.Choice.fst.checked \
+ /home/guido/r/everparse/main/src/cddl/pulse/CDDL.Pulse.Serialize.Gen.MapGroup.ZeroOrMore.Aux2.LoopBody.fst.checked \
src/cddl/pulse/CDDL.Pulse.AST.Ancillaries.fst.checked \
src/cbor/pulse/raw/CBOR.Pulse.API.Det.Rust.fst.checked \
- src/cddl/spec/CDDL.Spec.MapGroup.fst.checked \
src/cbor/pulse/raw/CBOR.Pulse.Raw.Format.Serialized.fsti.checked \
src/cddl/pulse/CDDL.Pulse.Serialize.Gen.MapGroup.ZeroOrMore.Aux2.Proof0.fsti.checked \
+ /home/guido/r/everparse/main/src/3d/prelude/EverParse3d.Prelude.fst.checked \
src/lowparse/LowParse.Low.DepLen.fst.checked \
- src/3d/prelude/EverParse3d.Kinds.fst.checked \
src/cbor/pulse/CBOR.Pulse.API.Det.C.Copy.fsti.checked \
src/lowparse/LowParse.Tot.List.fst.checked \
+ /home/guido/r/everparse/main/src/lowparse/LowParse.SLow.Int.fst.checked \
+ /home/guido/r/everparse/main/src/cddl/pulse/CDDL.Pulse.Serialize.Gen.MapGroup.ZeroOrMore.Aux2.Lemma8.fst.checked \
src/cddl/pulse/CDDL.Pulse.Parse.ArrayGroup.fst.checked \
src/lowparse/LowParse.Low.Tac.Sum.fst.checked \
src/cbor/spec/CBOR.Spec.Constants.fst.checked \
- src/lowparse/LowParse.Low.BoundedInt.fst.checked \
src/lowparse/LowParse.Tot.Base.fst.checked \
+ /home/guido/r/everparse/main/src/cddl/pulse/CDDL.Pulse.Serialize.Gen.MapGroup.ZeroOrMore.Aux2.Invariant.fst.checked \
src/lowparse/LowParse.Spec.Bytes.fst.checked \
+ /home/guido/r/everparse/main/src/3d/prelude/EverParse3d.ProbeActions.fst.checked \
+ /home/guido/r/everparse/main/src/cddl/pulse/CDDL.Pulse.Serialize.Gen.MapGroup.ZeroOrMore.Aux2.Lemma3.fst.checked \
src/cddl/pulse/CDDL.Pulse.AST.Env.fst.checked \
src/cddl/pulse/CDDL.Pulse.Serialize.Gen.MapGroup.Map.fsti.checked \
src/cbor/pulse/raw/CBOR.Pulse.Raw.Nondet.Compare.fst.checked \
@@ -24,15 +27,16 @@ ALL_CHECKED_FILES= \
src/cbor/spec/raw/CBOR.Spec.Raw.Format.MapPair.fsti.checked \
src/cddl/pulse/CDDL.Pulse.Serialize.Gen.MapGroup.ZeroOrMore.Aux2.Lemma8.fsti.checked \
src/cddl/pulse/CDDL.Pulse.Serialize.Gen.MapGroup.ZeroOrMore.Aux2.Lemma13.fsti.checked \
+ /home/guido/r/everparse/main/src/cddl/pulse/CDDL.Pulse.Serialize.Gen.MapGroup.ZeroOrMore.Aux2.Lemma6.fst.checked \
src/lowparse/LowParse.Spec.Combinators.fsti.checked \
src/lowparse/LowParse.Spec.AllIntegers.fst.checked \
src/cbor/pulse/raw/CBOR.Pulse.Raw.Read.fst.checked \
+ /home/guido/r/everparse/main/src/3d/prelude/EverParse3d.Kinds.fst.checked \
src/cbor/pulse/raw/everparse/CBOR.Pulse.Raw.EverParse.Nondet.Basic.fst.checked \
src/cbor/pulse/raw/everparse/CBOR.Pulse.Raw.EverParse.Format.fsti.checked \
src/cbor/pulse/raw/everparse/CBOR.Pulse.Raw.EverParse.Serialized.Base.fsti.checked \
src/lowparse/LowParse.SLow.BitSum.fst.checked \
src/cddl/pulse/CDDL.Pulse.AST.Types.fst.checked \
- src/lowparse/LowParse.SLow.Int.fst.checked \ |
|
I think the problem is this line added by the Dune PR: let all_cmd_line_files = expand_directories all_cmd_line_files inIn Everparse, the depend call is passed a long list of 412 files including fst's and fsti's. There are no directories to expand (as this is a new feature). However, the function unconditionally removes fsts for which an fsti was "found" (74 in total). Even with the patch here to respect fstar.include, the fsts get dropped, and rediscovered via the include path, and get an absolute path. Removing this line makes things work as before. I now understand Tahina's PR a bit better, and see how that fixes it. But I still don't understand why this filtering is here at all. Can anyone explain, or can we remove it? |
|
thanks for removing the filter! |
|
I removed the filtering. Everparse's .depend now looks as before AFAICT (there are many removals due to some modules of F*'s library being gone since my last build). examples/hello seems to work still. |
This removes conversion to absolute paths, and recursive directory traversal. It reverts to the previous fstar.include behavior.