ocamlPackages.janeStreet{,_0_9_0}: join the ocamlPackages fix point, allowing overriding to work as expected #113696
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
2f413da to
af2dbfd
Compare
|
Seems like I have to revert af2dbfd because The warnings stem from these bits: ppx_optcomp =
if lib.versionOlder "4.03" ocaml.version
then janeStreet.ppx_optcomp
else callPackage ../development/ocaml-modules/janestreet/ppx-optcomp.nix {};Alternatively we could either
Which also begs the question: Should we start removing some of the old janestreet stuff? Most packages in |
6a8147e to
ae7cd92
Compare
|
I've had a look at those conditional version changes and they are not that big of a problem:
However, the warning is still not feasible, |
|
@vbgl any thoughts on this? I have an idea on the deprecation idea in the previous commit by now, but I want to do it in a separate PR. |
ae7cd92 to
b201ef2
Compare
makes the diff larger and more awkward to read imo |
|
@ofborg eval |
Internal dependencies in the janeStreet sets were always taken from the
own rec attribute set. While this is pretty simple and convenient, it
has the disadvantage that it doesn't play nice with overriding: If you'd
override an attribute in a janeStreet set previously, it would be
changed when referenced directly, but the other packages in that
janeStreet set still would use the original, non-overridden version of
the derivation.
This is easily fixed by passing janeStreet_0_9_0 itself from the fix
point of ocamlPackages and using it to reference the dependencies.
Example showing it now works as expected:
test-overlay.nix:
self: super: {
ocamlPackages = super.ocamlPackages.overrideScope (old: _: {
janeStreet_0_9_0 = old.janeStreet_0_9_0 // {
base = old.janeStreet_0_9_0.base.overrideAttrs (_: {
meta.broken = true;
});
};
});
}
nix-repl> (import ./. {
overlays = [ (import ./test-overlay.nix) ];
}).ocamlPackages.janeStreet_0_9_0.stdio
error: Package ‘ocaml4.10.0-base-0.9.4’ in /home/lukas/src/nix/nixpkgs/pkgs/development/ocaml-modules/janestreet/janePackage.nix:6 is marked as broken, refusing to evaluate.
a) To temporarily allow broken packages, you can use an environment variable
for a single invocation of the nix tools.
$ export NIXPKGS_ALLOW_BROKEN=1
b) For `nixos-rebuild` you can set
{ nixpkgs.config.allowBroken = true; }
in configuration.nix to override this.
c) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
{ allowBroken = true; }
to ~/.config/nixpkgs/config.nix.
This change makes overrides to the janeStreet set work as expected by making the janeStreet set take part in the ocamlPackages fixpoint for janeStreet 0.14, i. e. OCaml >= 4.08
This change makes overrides to the janeStreet set work as expected by making the janeStreet set take part in the ocamlPackages fixpoint for janeStreet 0.12, i. e. OCaml == 4.07
This change makes overrides to the janeStreet set work as expected by making the janeStreet set take part in the ocamlPackages fixpoint for janeStreet 0.11, i. e. OCaml < 4.07
Previously, we inherited non-janestreet ocaml dependencies from super
and janestreet dependencies from self which always was super.janeStreet.
This behavior is however not really what we want due to liftJaneStreet:
Users and other packages will use ocamlPackages.base etc. instead of
ocamlPackages.janeStreet.base and the like. Consequently they also would
override the top-level attributes which would mean that other janestreet
packages would not pick up on it however.
As a consequence however, overriding ocamlPackages.janeStreet.base
doesn't work. Since this was never possible, I don't think this is an
issue. It is probably a good idea to deprecate that set anyways and
printing a warning when it is used via trace.
janeStreet_0_9_0 is unchanged as the disticniton between self and super
makes sense for it.
Below is an example showing how overriding would work from an user's
perspective:
test-overlay.nix:
self: super: {
ocamlPackages = super.ocamlPackages.overrideScope (old: _: {
base = old.base.overrideAttrs (_: {
meta.broken = true;
});
});
}
nix-repl> (import ./. { overlays = [ (import ./test-overlay.nix) ]; }).ocamlPackages.
stdio
error: Package ‘ocaml4.10.0-base-0.14.0’ in /home/lukas/src/nix/nixpkgs/pkgs/development/ocaml-modules/janestreet/janePackage_0_14.nix:12 is marked as broken, refusing to evaluate.
a) To temporarily allow broken packages, you can use an environment variable
for a single invocation of the nix tools.
$ export NIXPKGS_ALLOW_BROKEN=1
b) For `nixos-rebuild` you can set
{ nixpkgs.config.allowBroken = true; }
in configuration.nix to override this.
c) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
{ allowBroken = true; }
to ~/.config/nixpkgs/config.nix.
b201ef2 to
1a44809
Compare
sternenseemann
added a commit
to sternenseemann/nixpkgs
that referenced
this pull request
Jul 24, 2021
These packages have been failing to evaluate for some time now for two reasons: * ocamlmod doesn't evaluate for OCaml < 4.04 which is fixed in NixOS#113771 * We have no ppx_core version for OCaml < 4.03 which is a fundamental issue. Since this situation has been going on for some time now, I don't really see the need to figure out how to fix these legacy versions of some janestreet packages. There is also additional motivation: * These packages reference the janeStreet set directly which I'd like to deprecate, as it spells trouble for the overriding possibilities introduced in NixOS#113696. * These packages are patched in awkwardly into the janeStreet set without forming one themselves. By removing them we can move closer to having all janestreet packages managed uniformly in their own janestreet sets.
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.
Motivation for this change
Fix long standing
ocamlPackagesissue #75485: ThejaneStreetpackage set is maintained as a sub set ofocamlPackagesand merged into it usingliftJaneStreetin a second step. This is not an issue in itself, however inner-janestreet dependencies are done by using arecset in the respective nix expressions. This means thatjaneStreetpackages are oblivious to overrides being done inocamlPackagesto otherjaneStreetpackages and use the original derivation regardless.This is now fixed, see the commit message of ff40bbe for an example of this.
Another issue arising due to this is that
janeStreetpackages are both available viaocamlPackages.packageandocamlPackages.janeStreet.package. This creates an issue with overriding as overriding one the two attributes will always be ignored, as we only can use eitherocamlPackagesorocamlPackages.janeStreetas theselffix point to get dependencies from. Due toliftJaneStreetit feels more natural to use the top level attribute. To prevent confusion in the future we add a warning when usingocamlPackages.janeStreetinstead of the top level attributes (2f413da).cc @nomeata (original reporter) @vbgl
GitHub thingy: Resolves #75485.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)