ocamlPackages.janestreet: 0.15 -> 0.16 #247022
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This seems to break a few things (e.g., |
|
Sorry about that, you're right: more than a few packages are broken. I ran the following: I am slowly going through failures and patching where viable. I think I'm quite new to Nix, sorry about the failures again! Hate to waste reviewiers time. Regarding #230267, I was made aware of this PR after I had submitted this one. As far as I can see, the goal is the same. Seems like it's been inactive for a while. If there's a way to retrofit that PR with changes in this one, I'm happy to do so. |
|
Result of 5 packages failed to build:
44 packages built:
|
|
All packages except I fixed the hash for I also tested the build on x86_64-darwin. Could I get some help in making bap = callPackage ../development/ocaml-modules/bap {
inherit (pkgs.llvmPackages) llvm;
inherit (janeStreet_0_15) core_kernel ppx_jane;
};I believe the issue is that libraries in |
|
Everything except broken packages should build now. Here's the latest build report on x86_64-darwin: |
|
Awesome! The hash of |
|
Here is a patch to fix |
|
Fixed hash for I also moved |
|
Latest build report on x86_64-linux: @vbgl torch seems to build fine on tip, I left the tests enabled. |
|
Result of 51 packages built:
|
|
Tested a few of these packages manually to ensure things are working. Things look pretty good to me. |
Alizter
approved these changes
Aug 12, 2023
georgyo
approved these changes
Aug 12, 2023
ulrikstrid
approved these changes
Aug 15, 2023
There was a problem hiding this comment.
Seems ok, since @vbgl has been reviewing previously I'll let him merge
vbgl
reviewed
Aug 16, 2023
|
Result of 1 package marked as broken and skipped:
6 packages failed to build:
527 packages built:
|
vbgl
reviewed
Aug 17, 2023
| @@ -1836,6 +1871,7 @@ in let inherit (pkgs) callPackage; in rec | |||
| ocamlPackages_latest = ocamlPackages_5_0; | |||
|
|
|||
| ocamlPackages = ocamlPackages_4_14; | |||
| ocamlPackages_4_14_janeStreet_0_15 = ocamlPackages_4_14.overrideScope' (self: super: super // super.janeStreet_0_15); | |||
There was a problem hiding this comment.
Is this really needed? It does not look right.
There was a problem hiding this comment.
I needed this in order to build packages outside of ocamlPackages that depend on janestreet v0.15 libraries AND other non-JS OCaml packages, like stanc. With overrideScope', any non-JS OCaml modules that depend on JS libraries will pull in v0.15 libraries instead of the default v0.16 libraries. That way, external packages can use both JS and non-JS libraries without accidentally pulling in both v0.15 and v0.16 due to transitive build dependencies. Simply plug in ocamlPackages_4_14_janeStreet_0_15 as ocamlPackages in stanc derivation.
The idea is to override attributes lifted one level up from janeStreet (v0.16) by first call to overrideScope', with attributes from janeStreet_0_15 (v0.15).
Another way of dealing with this is carefully and incrementally crafting transitive dependencies with .override to use v0.15, like I had to do for e.g. bap. But this seems quite messy.
It's nasty, I agree. I reckon we can get rid of this as soon as v0.16 libraries are supported upstream.
Any cleaner solutions to this are very welcome!
There was a problem hiding this comment.
Alright. If we agree on the fact that this is a temporary ugly hack, then that’s fine. Let’s hope stanc and ligo get updated.
|
@GrahamcOfBorg build ligo |
|
Evaluation seems broken. |
|
Result of |
Working on getting these to compile. |
|
Evaluation now builds. All packages affected by the PR now build on x86_64-linux -- tested with Due to changes to
Kindly review the changes. |
|
Output of |
vbgl
approved these changes
Aug 30, 2023
|
Yep, that's a good idea. I'll do it tonight when I'm back home. |
vbgl
reviewed
Aug 31, 2023
|
Packages added in #230267 but not here:
|
|
Three of those missing packages have just been added in #252614. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add version 0.16 of Jane Street OCaml libraries. Note that this does not replace the existing 0.15 set of libraries.
Notable omissions:
ocamlPackages.torch. Recently Jane Street took over development of said library. At the moment, both old and new version of the library are still in use, because the new version does not support PyTorch 2.0.0. The new, JS-maitained torch library should be added underocamlPackages.torch, and is out of scope of this PR.Other notes:
Description of changes
Release announcement: https://discuss.ocaml.org/t/ann-v0-16-release-of-jane-street-packages/12398
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)