Skip to content
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

Upgrades BAP to janestreet v0.16 libraries #1602

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bmourad01
Copy link
Contributor

@bmourad01 bmourad01 commented Apr 13, 2024

Related: #1589

This is here as a placeholder for now in case we want to upgrade to v0.16.0 version of base, core, core_kernel, and other related libraries. It is desirable to keep the version of janestreet libraries (such as base, core, and core_kernel) as current as possible.

Several interfaces have been changed/broken as far as I can see, namely:

  • X.Set, X.Map, and X.Table have been gutted of various functions that are present in Set, Map, and Hashtbl, respectively
  • The Skip and Yield constructors for Sequence.Step.t each expect an inline record for their arguments.
  • Binable.Of_binable, Binable.Of_stringable, and Bin_prot.Make_binable are gone and now have {with,without}_uuid and other variants. The without_uuid is marked as "legacy" in the compiler warnings, stating that with_uuid is preferred. I'm leaving it as the former for now as it doesn't require us to change anything else for these functor applications.
  • Caml is now deprecated in favor of Stdlib

Right now some of the PowerPC unit tests are failing on my end, so I will try to investigate later when I have time (actually, it seems that they are failing without this change. I'm not sure if this is due to a change in newer versions of LLVM or something else). See #1603, can be merged independently.

@bmourad01 bmourad01 force-pushed the upgrade-janestreet-16 branch from 6a873d1 to c172d62 Compare April 13, 2024 23:41
@bmourad01
Copy link
Contributor Author

@ivg pinging just to make you aware of this

ivg
ivg previously approved these changes Apr 23, 2024
Copy link
Member

@ivg ivg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through all the changes, they look pretty syntactical. I wonder what could get wrong that ppc tests fail.

@XVilka
Copy link
Contributor

XVilka commented Dec 14, 2024

@bmourad01 do you plan to update/finish the PR?

@bmourad01 bmourad01 force-pushed the upgrade-janestreet-16 branch from c172d62 to 69b3408 Compare December 17, 2024 01:43
@bmourad01 bmourad01 marked this pull request as ready for review December 17, 2024 01:43
@bmourad01
Copy link
Contributor Author

bmourad01 commented Dec 17, 2024

@bmourad01 do you plan to update/finish the PR?

I believe it should be ready now. I put extra constraints on the versions of the libraries (we don't want to allow v0.17 out of the gate, as it will exclude OCaml 4).

Later in the future we can have more discussion about what needs to be done to move BAP to be ready for OCaml 5 (but it seems that this would mean abandoning support for OCaml 4).

@bmourad01 bmourad01 changed the title Upgrades BAP to janestreet v0.16.0 libraries Upgrades BAP to janestreet v0.16 libraries Dec 17, 2024
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.

3 participants