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

Jump to bottom

ocamlPackages.cohttp-async: init at 2.5.1 #88886

Merged
merged 5 commits into from Aug 29, 2020
Merged

ocamlPackages.cohttp-async: init at 2.5.1 #88886

merged 5 commits into from Aug 29, 2020

Conversation

bcc32
Copy link
Contributor

@bcc32 bcc32 commented May 25, 2020 •
edited

Motivation for this change

Add cohttp-async, upgrading cohttp and conduit to make this possible.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@bcc32 bcc32 changed the title Init cohttp-async @ 2.5.1, upgrading cohttp 2.1.3 -> 2.5.1 cohttp-async: init at 2.5.1 May 25, 2020
@bcc32 bcc32 changed the title cohttp-async: init at 2.5.1 ocamlPackages.cohttp-async: init at 2.5.1 May 25, 2020
@bcc32 bcc32 marked this pull request as ready for review May 25, 2020 18:27
@sternenseemann
Copy link
Member

You might want to rebase this change against master. conduit, ipaddr, macaddr, tuntap and cohttp have been updated (cohttp is on 2.5.1).

@bcc32
Copy link
Contributor Author

bcc32 commented Jun 29, 2020

Thanks for the heads up, this PR is much cleaner now :)

@sternenseemann sternenseemann requested a review from vbgl June 30, 2020 09:00
sternenseemann
sternenseemann requested changes Jun 30, 2020
View reviewed changes
Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

Builds fine for me. A bit of tidying up would be nice though:

  • efa7f1dabf4fb591a3943450a4b15369fd6819b9 has a bit of a messy diff seems like some unnecessary indentation changes got commited. Also use the nixpkgs commit message format
  • split 8baf77bf2fd1a0a9c3e8b4a21fb5ab6a6ee74c7e into three commits adding the individual packages.

@bcc32
Copy link
Contributor Author

bcc32 commented Jul 10, 2020

Thanks for the feedback! (I'm still new to nix so a lot of my edits have been guess-work and pattern matching.)

I've split the commit into one for each package. Do you think it would be good to also squash e76ba09 into d56e12e, or is it fine to keep them separate?

sternenseemann
sternenseemann requested changes Jul 10, 2020
View reviewed changes
Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

Commits should be fine like current, but you can also squash the two if you want.

pkgs/development/ocaml-modules/janestreet/0.13.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/cohttp/async.nix Outdated Show resolved Hide resolved
pkgs/development/ocaml-modules/conduit/async.nix Show resolved Hide resolved
pkgs/development/ocaml-modules/cohttp/async.nix Outdated
useDune2 = true;
inherit (cohttp) version src meta;

buildInputs = [ uri ppx_sexp_conv ];
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't be uri in propagatedBuildInputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modeled this off of cohttp/lwt.nix. I'm not especially clear on the difference, so I didn't make any changes.

sternenseemann
sternenseemann requested changes Jul 25, 2020
View reviewed changes
Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

otherwise lgtm

pkgs/development/ocaml-modules/cohttp/async.nix Outdated Show resolved Hide resolved
@bcc32
Copy link
Contributor Author

bcc32 commented Jul 25, 2020

Awesome. Thanks for the careful review and the helpful feedback!

@sternenseemann sternenseemann mentioned this pull request Jul 26, 2020
Merged
10 tasks
@sternenseemann
Copy link
Member

Can you test this against cb2d328?

@bcc32
Copy link
Contributor Author

bcc32 commented Aug 15, 2020

I just rebased against master, and everything seems to be in working order (nix build -f . ocamlPackages.cohttp-async, nixpkgs-review pr 88886).

@vbgl
Copy link
Contributor

vbgl commented Aug 23, 2020 •
edited

@GrahamcOfBorg build ocaml-ng.ocamlPackages_4_07.cohttp-async

vbgl
vbgl requested changes Aug 23, 2020
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.

It seems that cohttp-async won’t build with OCaml 4.07. Can you please set its minimumOCamlVersion to 4.08? Thanks.

@bcc32
Copy link
Contributor Author

bcc32 commented Aug 28, 2020

I think this is an issue of a change that happened in Jane Street's async packages (Tcp module was moved from Async_extra to Async_unix). Is there a way I can require that the version of the async package is at least 0.13?

@vbgl
Copy link
Contributor

vbgl commented Aug 28, 2020

There is such a check in bap:

nixpkgs/pkgs/development/ocaml-modules/bap/default.nix

Lines 9 to 11 in 1c243af

if stdenv.lib.versionAtLeast core_kernel.version "0.12"
then throw "BAP needs core_kernel-0.11 (hence OCaml ≤ 4.06)"
else

@bcc32
Copy link
Contributor Author

bcc32 commented Aug 28, 2020

Thanks, @vbgl. I added such a check to cohttp-async.

@vbgl vbgl merged commit 5cf4bb8 into NixOS:master Aug 29, 2020
@bcc32 bcc32 deleted the cohttp-async branch August 29, 2020 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vbgl vbgl vbgl approved these changes

@sternenseemann sternenseemann sternenseemann approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bcc32 @sternenseemann @vbgl