-
Notifications
You must be signed in to change notification settings - Fork 236
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
Staged build, new CI, new packaging #3637
Conversation
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.
The F# files are all renamed to XX_*
. Does this PR drop support for F# extraction?
.docker/dev-base.Dockerfile
Outdated
# currently also have 4.8.5 in the opam switch, as it is a dependency of | ||
# in fstar.opam, but that should be removed. | ||
|
||
RUN wget -nv https://github.com/Z3Prover/z3/releases/download/Z3-4.8.5/z3-4.8.5-x64-ubuntu-16.04.zip \ |
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.
This should use the new get_fstar_z3.sh script.
.github/workflows/ci.yml
Outdated
- name: Checkout | ||
uses: actions/checkout@master | ||
|
||
- name: Check for a stage 3 diff |
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.
Should we append this step to build to reduce CI usage?
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.
I was wondering how best to split all this.. doing that does save on total usage, but splitting it away reduces the time until the test jobs can start. (Testing could also be done in this same job, which would maximize parallelism with stage3, but I liked the idea of a starting from a clean slate, and allows subprojects to be checked even if some test fails.) Another bonus is that if the stage3 diff fails for some reason, we still go ahead with normal tests.
Maybe we can just record the state of the repo in an artifact (in fact it's done already, for Pulse) and start the stage3 check from there... wdyt?
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.
Maybe we can just record the state of the repo in an artifact (in fact it's done already, for Pulse) and start the stage3 check from there... wdyt?
That's probably the best idea.
.github/workflows/ci.yml
Outdated
# them is created, and start the subsequent jobs at that instant too. | ||
# Is that even doable...? | ||
|
||
check-friends: |
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.
Should we really build the whole world on every push?
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.
No, that's just temporary while we work on this. I think the consensus is to run internal F* tests to allow merges to dev
, and a world check for master
.
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.
Let's not forget to turn it off before merging then!
ln -Trsf stage$*/out out | ||
# For compatibility with the previous layout | ||
mkdir -p bin | ||
ln -Trsf out/bin/fstar.exe bin/fstar.exe |
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.
I still think it's a bad idea to make the default F* binary point to whatever make target succeeded last, and this will likely confuse the hell out of me.
It's great that you changed the ulib vscode config to point to stage1 directly (and src to stage0). That addresses most of my practical concerns with debugging failing builds.
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.
I tend to agree, but I think we'll want a way to be able to run the tests on the stage 1 compiler. Currently that's as easy as make 1 && make test
. Note that the out
link is only created when you call make 1
or make 2
. Most users will not use these rules and just get the stage 2 compiler, i.e. the actual final product.
I'd also be happy having a differently named rule for setting the link to the stage 1, fake-1
or whatever.
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.
I think the most natural way to run tests is make test
; and I'd expect that it would be roughly similar to CI, i.e., it should build F* and run the tests on stage 2.
I also just realized that make test
by itself would fail (or use an out-of-date fstar.exe) since it doesn't depend on any build target. That's really unexpected.
Another options is to have test-1
and test-2
targets, and maybe test: test-2
for convenience. (Then the tests could also use different cache directories for each stage if we want to support that.)
Most users will not use these rules
What you're saying is that if you only ever call make
, then this defaults to make 2
and you'll only ever get a symlink to stage 2, right?
Is there a target to build stage N without messing with the symlinks?
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.
I think the most natural way to run tests is
make test
; and I'd expect that it would be roughly similar to CI, i.e., it should build F* and run the tests on stage 2.I also just realized that
make test
by itself would fail (or use an out-of-date fstar.exe) since it doesn't depend on any build target. That's really unexpected.Another options is to have
test-1
andtest-2
targets, and maybetest: test-2
for convenience. (Then the tests could also use different cache directories for each stage if we want to support that.)
I changed the rules to roughly do this (see last patch). I kept a make _test
available that will just use the latest one, since I think I will be running this often when working with a stage 1 (to avoid the overhead of dune realizing there's nothing to do).
Most users will not use these rules
What you're saying is that if you only ever call
make
, then this defaults tomake 2
and you'll only ever get a symlink to stage 2, right?
Yes exactly.
Is there a target to build stage N without messing with the symlinks?
I added stage1
and stage2
to build the stages without setting the out/
link. The shortcuts 1
and 2
call the respective stage target and then set the link.
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.
SGTM. Thank you!
@@ -0,0 +1,102 @@ | |||
open FStarC_Compiler_Range |
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.
These files (hand-written ML, F*, ulib, etc.) all show up in the git diff. Should we mark them as binaries like the checked in ML files?
64240b7
to
599f5d2
Compare
When I run
|
Thanks for the fix! Next thing: |
Another weird thing: if I have a file |
Ah thanks! That's a consequence of implicitly adding |
a184774
to
3713c56
Compare
Makefile
Outdated
3: | ||
+$(Q)$(MAKE) 1 | ||
+$(Q)$(MAKE) 2 | ||
SHELL=/bin/bash |
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.
SHELL=/bin/bash | |
SHELL=bash |
Is there a reason we need to hardcode the full path to bash here?
As you've observed, this doesn't work in nixpkgs. More problematically, it doesn't work on NixOS either (where you'd need to write make SHELL=bash
everytime you call make..).
(EDIT: Changed my suggestion to keep the SHELL
but set it to bash
instead of the full path. I just realized that you added this because of Debian, which defaults to dash
.)
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.
I think there's no need for bash at all actually, pushed a patch.
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.
Commented before seeing the edit, does something fail in dash
? I think I was being too defensive by using bash since I had many complicated rules, but that logic has been pushed to some scripts (which do require bash, but the Makefile does not afaict).
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.
I didn't try to build on Debian, I was just figuring out the reason why you put the SHELL=
there in the first place.
If you've already removed all the bashisms from the Makefile, then we're good now.
I'm not sure where best to put this tip, but with the new symlink forest it is super useful to set Alternatively, we could also add these directories to |
I added a macos build and a release workflow. Some minor points to figure out are:
|
There seem to be some missing dependencies. If you switch between this PR and the git checkout mtzguido/dev
make -j`nproc` stage2
git checkout gebner_smt_univs
make -j`nproc` stage2
# lots of error messages that result from mixing old and new SMT encoding It works after a |
Another QoL issue. Could we rename the executable to
|
I think the missing dependency is these checked files depending on For changes that affect the contents of checked files we usually bump the version number in in Prims.fst and FStarC.CheckedFiles. Touching prims will force rebuilds. If you add a change like this to that branch does everything work fine? |
That sounds practical. Thank you for the explanation. With the missing dependency you can safely edit any
Yes, I could touch |
@mtzguido Thanks for renaming the executable to |
Still a draft, posting for visibiblity
A mostly green local + friends CI here: https://github.com/mtzguido/FStar/actions/runs/12359614442/attempts/1
Missing:
Nix build