-
Notifications
You must be signed in to change notification settings - Fork 63
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
Simplify Makefiles after F* changes #536
Conversation
Wondering if |
1- yeah I was just about to suggest the same thing it's weird to have specific options for specific purposes -- most tools use -o regardless of the type of file produced (e.g. gcc -E, gcc -MD, gcc -c, gcc -shared, produce .c, .d, .o, .so but all are controlled by -o) |
For 2) yes I don't think we're testing the multi-file invocations... not sure if they work reliably. But not sure I understand what you mean about the stubs, can't krml just pass |
I guess? Like krml would invoke F* with |
Yeah exactly, that should make F* behave just as it does now. I'll give it a go and update here, it'd be nice to make this consistent. |
Updated after implementing |
$(FSTAR) $(notdir $(subst .checked,,$<)) --codegen krml \ | ||
--extract_module $(basename $(notdir $(subst .checked,,$<))) | ||
$(FSTAR) $< --codegen krml \ | ||
--extract_module $(basename $(notdir $(subst .checked,,$<))) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this one need extract_module but not the one above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--codegen krml still has the default extracting everything (instead of just the module in cmdline). I'll change that soon in a separate PR as it'd be a breaking change for many projects (I think)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok sounds good -- uniformity is great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me minus my comments. Thanks for this!
test/sepcomp/a/Makefile
Outdated
.PRECIOUS: obj/%.ml | ||
obj/%.ml: | ||
$(FSTAR) $(notdir $(subst .checked,,$<)) --codegen OCaml \ | ||
$(FSTAR) $< --codegen OCaml \ | ||
--extract_module $(basename $(notdir $(subst .checked,,$<))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then should this extract_module go? (per your comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, updated.
Mostly: 1. For extracting, we can just pass the path of the checked file to F* instead of doing the hacky substitution and rely on F* to find the source module. 2. no need to pass --extract when we want to extract a single file (except for --codegen krml, separate PR incoming for that). 3. Using -o instead of (the now deprecated) --output_deps_to and --krmloutput. 4. Using -c instead of --cache_checked_modules, and only passing it when we are actually trying to create a checked file. Also added some explicit -o flags to make sure F* writes to the desired target.
Mostly:
F* instead of doing the hacky substitution and rely on F* to find the
source module.
(except for --codegen krml, separate PR incoming for that).
--krmloutput.
when we are actually trying to create a checked file.
Also added some explicit -o flags to make sure F* writes to the desired
target.
(Previous text:
Mostly no need to pass --extract when we want to extract a single file, and we can just pass the path of the checked file to F* instead of doing the substitution.
Also added some explicit --krmloutput flags to make sure F* writes to the desired target.)