-
Notifications
You must be signed in to change notification settings - Fork 274
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
Try to build with OCaml 4.08 version #957
Conversation
Strange error on 4.07 compiler:
Also I don't get what is happening there - error message looks empty
|
Blocked by alavrik/piqi#62 and camlp4/camlp4#151 |
I checked the rest of the dependencies - they behave well for OCaml 4.08.0+, so once piqi is fixed, should be good. |
Note that camlp5 7.08 was released, and piqi moved to use it instead of camlp4. So please rerun the Travis, to check the build. |
Our Travis infrastructure is still relying on opam 1.x so we don't see the updated piqi package. That only means, that we have to update our Travis CI scripts. Indeed, it doesn't make sense to test installability with 1.x |
I don't get the reason of 4.07 build failure:
|
The build fails, because the oasis file wasn't updated (it also limits the version of OCaml). @gitoleg revived his old branch and managed to build everything for 4.08. While he is running his tests, here is the status update. The 4.07 is the latest version supported by core_kernel.v0.11.0, so in order to switch to 4.08 we need to switch to a newer version of core_kernel. As always, the switch is not backward compatible, so we can't support both v0.11 and v0.12 (especially, since they renamed a couple of libraries). Therefore, the solution is to switch to v0.12 and drop the support for 4.04, 4.05, and 4.06. So that now we will support only 4.07 and 4.08. I'm totally ready to switch to 4.08+core.v0.12 even today in bap-2.0 branch. The main question is whether we should backport this to 1.x. Basically, the list of options that we have:
Opinions, other options? |
I think the third option is the sanest one, keeping one with the modern world, without the breaking old things. |
Yep, but this implies lots of work and less clear work. If we will pursue this path, then we need to keep code compatible with different versions of OCaml (including the deprecation and removal of Pervasives and Genarray.*.map_file, which requires us to add a few shim libraries as dependencies, and use them as indirections in our code). |
Then the least work path
…On Wed, Aug 14, 2019, 12:59 AM Ivan Gotovchits ***@***.***> wrote:
I think the third option is the sanest one, keeping one with the modern
world, without the breaking old things.
Yep, but this implies lots of work and less clear work. If we will pursue
this path, then we need to keep code compatible with different versions of
OCaml (including the deprecation and removal of Pervasives and
Genarray.*.map_file, which requires us to add a few shim libraries as
dependencies, and use them as indirections in our code).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#957?email_source=notifications&email_token=AABRT7K7CNJWXQ5MLD3JNN3QELSAVA5CNFSM4HZFZ3K2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4GJSPQ#issuecomment-520919358>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABRT7OPTWO7EUSHQTMBSZ3QELSAVANCNFSM4HZFZ3KQ>
.
|
This branch is go to go into the master (after a merge or rebase), see the related issue for more discussion |
Rebased against the current master. Travis is drunk though. Sometimes it shows "Can't find the repository", sometimes - "can't find the build". |
I don't get why it shows while it was fixed:
Even the line number is wrong |
Looks like Travis doesn't get the valid sources. |
Note, that the piqi switched to sedlex, so no need for camlp4 and camlp5 dependency! \o/ |
I think we also can drop these lines from the case "$OCAML_VERSION,$OPAM_VERSION" in
3.12,1.2.2) OCAML_FULL_VERSION=3.12.1; brew install opam ;;
4.00,1.2.2) OCAML_FULL_VERSION=4.00.1; brew install opam ;;
4.01,1.2.2) OCAML_FULL_VERSION=4.01.0; brew install opam ;;
4.02,1.2.2) OCAML_FULL_VERSION=4.02.3; brew install opam ;;
4.02,1.3.0) OCAML_FULL_VERSION=4.02.3; brew install opam --HEAD ;; https://github.com/BinaryAnalysisPlatform/bap/blob/master/.travis-ocaml.sh#L142 |
There is still no answer from piqi author, but maybe it's possible to pin the repo for a while? |
Should be good to retry once ocaml/opam-repository#14947 is merged: ChangesPiqi 0.6.15 (September 28, 2018) Miscellaneous:
P.S. We should try also the 4.09 too. |
Still the same error. Travis seems out of sync with the git and opam. |
So what do we do with this one? |
@XVilka |
@ivg completely agree. One of the interesting features of the new library is a deriver plugin for Python accessors generation: https://github.com/janestreet/ppx_python Since BAP has Python bindings, it might be quite useful for making them better. |
lib/bap_future/bap_future.mli
Outdated
module Args : Applicative.Args with type 'a arg := 'a t | ||
|
||
module Args : sig | ||
type 'a arg = 'a t |
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.
remove the arg type from the signature, it wasn't there before
List.for_all ~f:(fun (v,x) -> | ||
f v.data.typ x) | ||
|
||
let zip x y = | ||
let open List.Or_unequal_lengths in |
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.
There is no need for this open, as now the compiler will take the constructors from the right place.
@@ -178,15 +178,15 @@ let pp_generic | |||
| `this x -> fprintf ppf "%s" x | |||
| `base -> pp_prefix ppf | |||
| `auto -> | |||
if Bitvec_order.(x >= (min (word 10) base)) | |||
if Z.Compare.(x >= (Z.min (word 10) base)) |
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.
wow, it was a bug before... not that it was breaking before, but at least the code as written was not the code as meant.
lib/bap_types/bap_ir.ml
Outdated
let apply_map name get map skip blk ~f = | ||
if List.mem ~equal:Polymorphic_compare.equal skip name | ||
let apply_map ~equal name get map skip blk ~f = | ||
if List.mem ~equal skip name |
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.
it is fine to use the polymorphic compare here
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.
a couple of changes and we're good to go
- restore
ignore
instead ofCaml.ignore
. - remove the _ arg type from the Args interface
As a general note, it is fine to use polymorphic compare, so this PR could be much easier to digest if we would just do open Poly
.
File "oasis/common.setup.ml.in", line 1773, characters 22-40: Alert deprecated: module Stdlib.Pervasives Use Stdlib instead. If you need to stay compatible with OCaml < 4.07, you can use the stdlib-shims library: https://github.com/ocaml/stdlib-shims File "setup.ml", line 3467, characters 16-34: 3467 | Pervasives.compare o2 o1) ^^^^^^^^^^^^^^^^^^ Alert deprecated: module Stdlib.Pervasives Use Stdlib instead. If you need to stay compatible with OCaml < 4.07, you can use the stdlib-shims library: https://github.com/ocaml/stdlib-shims
Previously, we decided to disable the Jane Street code style checks as a fast solution, but it turned out that it is easier to adopt their style than to fight with it. To disable the checks, we used the _tags file, but since we have a global repository the tags file affected all subprojects, even those that do not use the ppx-jane preprocessor, thus breaking packages that do not explicitly depend on ppx-jane. Also, in BinaryAnalysisPlatform#957 we accidentally introduced a dependency on a newer version of `zartith` by using the `Compare` module. We will now use plaincompare et alas functions so that the old version of `zarith` could be still used with BAP.
Previously, we decided to disable the Jane Street code style checks as a fast solution, but it turned out that it is easier to adopt their style than to fight with it. To disable the checks, we used the _tags file, but since we have a global repository the tags file affected all subprojects, even those that do not use the ppx-jane preprocessor, thus breaking packages that do not explicitly depend on ppx-jane. Also, in #957 we accidentally introduced a dependency on a newer version of `zartith` by using the `Compare` module. We will now use plaincompare et alas functions so that the old version of `zarith` could be still used with BAP.
With the new OCaml 4.08 release there are lot of nice optimizations and features out of the box, even without changing the current code. Worth to make sure it builds. For now lets see if everything passes OK.