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.

Sign up for GitHub

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

Jump to bottom

ocamlPackages: fix broken packages #162745

Merged
merged 2 commits into from
Mar 25, 2022
Merged

ocamlPackages: fix broken packages #162745

merged 2 commits into from
Mar 25, 2022

Conversation

ulrikstrid
Copy link
Member

Motivation for this change

Fix a lot of packages broken by #161344 and some extras I found along the way. The commits are structured so that we don't have to squash the commits if we don't want to.

ppx_deriving was intentionally left out of this since there's another open PR for it.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ulrikstrid ulrikstrid requested review from vbgl and symphorien March 4, 2022 10:37
@ulrikstrid ulrikstrid force-pushed the ulrikstrid/fix-ocaml-packages branch from a181ab0 to 701c0cc Compare March 4, 2022 11:38
@ulrikstrid
Copy link
Member Author

Fixed and rebased after your review @SuperSandro2000

@ulrikstrid ulrikstrid force-pushed the ulrikstrid/fix-ocaml-packages branch from 5ecd248 to d181c3a Compare March 4, 2022 14:24
@ulrikstrid ulrikstrid changed the title ocamlPackages: fix broken packages. ocamlPackages: fix broken packages Mar 4, 2022
@ulrikstrid ulrikstrid mentioned this pull request Mar 4, 2022
Merged
13 tasks
@ulrikstrid
Copy link
Member Author

ulrikstrid commented Mar 6, 2022 •
edited
Loading

@vbgl I'm pretty sure this PR fixes the rest of the broken packages you found

vbgl
vbgl requested changes Mar 6, 2022
View reviewed changes
Copy link
Contributor

@vbgl vbgl left a comment

Choose a reason for hiding this comment

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

Might be easier to review if split into smaller independent pieces.

pkgs/build-support/ocaml/dune.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/owee/default.nix Outdated Show resolved Hide resolved
@ulrikstrid
Copy link
Member Author

@vbgl each commit is self contained, if you would rather have me move the commits into separate PRs I can do that

@SuperSandro2000
Copy link
Member

@vbgl each commit is self contained, if you would rather have me move the commits into separate PRs I can do that

I think it is fine how it is with one PR and multiple commits. We don't need 6 PRs for those fixes.

@vbgl
Copy link
Contributor

vbgl commented Mar 11, 2022

Right, we don’t need 6 PRs. But the first commit is controversial and blocks all the rest.

@ulrikstrid ulrikstrid mentioned this pull request Mar 12, 2022
Closed
13 tasks
@ulrikstrid ulrikstrid force-pushed the ulrikstrid/fix-ocaml-packages branch from d181c3a to 94a9a2d Compare March 12, 2022 06:20
@ulrikstrid
Copy link
Member Author

@vbgl I moved the buildDunePackages commit to #163766

vbgl
vbgl reviewed Mar 13, 2022
View reviewed changes
pkgs/development/ocaml-modules/ppx_deriving_protobuf/default.nix
buildInputs = [ cppo ppxlib ppx_deriving ];
nativeBuildInputs = [ cppo ];
buildInputs = [ ppxlib dune-configurator ];
propagatedBuildInputs = [ ppx_deriving ];
Copy link
Contributor

@vbgl vbgl Mar 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This looks wrong (why is this dependency propagated?).

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it needs the runtime library in consuming packages.

@ulrikstrid ulrikstrid force-pushed the ulrikstrid/fix-ocaml-packages branch from 94a9a2d to bc70a64 Compare March 13, 2022 12:06
@vbgl vbgl mentioned this pull request Mar 13, 2022
Merged
13 tasks
@ulrikstrid ulrikstrid force-pushed the ulrikstrid/fix-ocaml-packages branch 3 times, most recently from 0badf27 to 1f62f77 Compare March 14, 2022 07:08
@ulrikstrid
Copy link
Member Author

@vbgl I rebased this on master, if the facile/nonstd commit is too controversial I can just drop it from here.

@ulrikstrid
Copy link
Member Author

ping @vbgl

vbgl
vbgl reviewed Mar 19, 2022
View reviewed changes
pkgs/development/ocaml-modules/angstrom-async/default.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/caqti/default.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/jwto/default.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/ocb-stubblr/default.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/ocp-ocamlres/default.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/pipebang/default.nix
@@ -15,7 +15,7 @@ buildOcaml rec {

strictDeps = true;

buildInputs = [ camlp4 ];
propagatedBuildInputs = [ camlp4 ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider dropping this package altogether?

Copy link
Member Author

Choose a reason for hiding this comment

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

Dropping pipebang?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I just revert this and then create a separate PR dropping pipebang? I feel like I don't want to bundle it into this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or, we could keep it and open a separate PR either way

@ulrikstrid ulrikstrid force-pushed the ulrikstrid/fix-ocaml-packages branch 4 times, most recently from 6f4714d to 220d1ec Compare March 20, 2022 06:22
@ulrikstrid ulrikstrid requested a review from vbgl March 20, 2022 06:24
@ulrikstrid
Copy link
Member Author

@vbgl Thank you for your review 🙏 I think there's just 1 open question now, I'm leaning to fix pipebang now and then remove it in a separate PR.

@ulrikstrid ulrikstrid force-pushed the ulrikstrid/fix-ocaml-packages branch from 220d1ec to cfb23f6 Compare March 20, 2022 16:13
vbgl
vbgl reviewed Mar 21, 2022
View reviewed changes
Copy link
Contributor

@vbgl vbgl left a comment

Choose a reason for hiding this comment

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

Please properly fix type_conv or drop the changes to these packages (the issue can be addressed in a later PR).

pkgs/development/ocaml-modules/type_conv/108.08.00.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/type_conv/109.60.01.nix Outdated Show resolved Hide resolved
@ulrikstrid ulrikstrid force-pushed the ulrikstrid/fix-ocaml-packages branch from cfb23f6 to 6d2c62d Compare March 24, 2022 20:03
@ulrikstrid ulrikstrid requested a review from vbgl March 24, 2022 20:03
@siraben
Copy link
Member

siraben commented Mar 25, 2022

Result of nixpkgs-review pr 162745 run on aarch64-darwin 1

3 packages marked as broken and skipped:
  • ikos
  • ocamlPackages.wodan-irmin
  • opa
22 packages failed to build:
  • ocamlPackages.arp
  • ocamlPackages.cohttp-mirage
  • ocamlPackages.conduit-mirage
  • ocamlPackages.elina
  • ocamlPackages.git-cohttp-unix
  • ocamlPackages.git-paf
  • ocamlPackages.git-unix
  • ocamlPackages.irmin-containers
  • ocamlPackages.irmin-git
  • ocamlPackages.irmin-http
  • ocamlPackages.irmin-mirage-git
  • ocamlPackages.irmin-mirage-graphql
  • ocamlPackages.irmin-unix
  • ocamlPackages.mirage-nat
  • ocamlPackages.paf
  • ocamlPackages.paf-cohttp
  • ocamlPackages.paf-le
  • ocamlPackages.tcpip
  • ocamlPackages.tezos-mockup-proxy
  • ocamlPackages.tezos-proxy
  • ocamlPackages.tezos-rpc-http-client-unix
  • ocamlPackages.tezos-signer-backends
52 packages built:
  • comby
  • framac
  • ocamlPackages.apron
  • ocamlPackages.cohttp-async
  • ocamlPackages.cohttp-lwt-unix
  • ocamlPackages.conduit
  • ocamlPackages.conduit-async
  • ocamlPackages.conduit-lwt
  • ocamlPackages.conduit-lwt-unix
  • ocamlPackages.containers
  • ocamlPackages.containers-data
  • ocamlPackages.eliom
  • ocamlPackages.ethernet
  • ocamlPackages.facile
  • ocamlPackages.graphql-cohttp
  • ocamlPackages.hack_parallel
  • ocamlPackages.irmin-graphql
  • ocamlPackages.iter
  • ocamlPackages.letsencrypt-app
  • ocamlPackages.mccs
  • ocamlPackages.mdx
  • ocamlPackages.mirage-logs
  • ocamlPackages.mirage-profile
  • ocamlPackages.mirage-vnetif
  • ocamlPackages.mlgmpidl
  • ocamlPackages.nonstd
  • ocamlPackages.ocsigen-start
  • ocamlPackages.ocsigen-toolkit
  • ocamlPackages.ocsigen_server
  • ocamlPackages.odoc-parser
  • ocamlPackages.opium
  • ocamlPackages.opium_kernel
  • ocamlPackages.pcap-format
  • ocamlPackages.phylogenetics
  • ocamlPackages.pipebang
  • ocamlPackages.ppx_cstubs
  • ocamlPackages.ppx_deriving_protobuf
  • ocamlPackages.ppx_deriving_yaml
  • ocamlPackages.printbox
  • ocamlPackages.printbox-text
  • ocamlPackages.resto-cohttp-self-serving-client
  • ocamlPackages.resto-cohttp-server
  • ocamlPackages.shared-memory-ring
  • ocamlPackages.shared-memory-ring-lwt
  • ocamlPackages.tsort
  • ocamlPackages.ulex
  • ocamlPackages.yaml
  • ocamlPackages.yaml-sexp
  • ocamlformat (ocamlformat_0_20_1)
  • ocamlformat_0_19_0
  • ocamlformat_0_20_0
  • soupault

siraben
siraben approved these changes Mar 25, 2022
View reviewed changes
Copy link
Member

@siraben siraben left a comment

Choose a reason for hiding this comment

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

LGTM

@vbgl vbgl merged commit d6e2e39 into NixOS:master Mar 25, 2022
@ulrikstrid ulrikstrid deleted the ulrikstrid/fix-ocaml-packages branch March 25, 2022 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siraben siraben siraben approved these changes

@symphorien symphorien Awaiting requested review from symphorien

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@vbgl vbgl Awaiting requested review from vbgl

Assignees
No one assigned
Labels
6.topic: ocaml 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ulrikstrid @SuperSandro2000 @vbgl @siraben