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

dune: init at 2.1.0 #74261

Merged
merged 12 commits into from Dec 23, 2019
Merged

dune: init at 2.1.0 #74261

merged 12 commits into from Dec 23, 2019

Conversation

marsam
Copy link
Contributor

@marsam marsam commented Nov 26, 2019

Motivation for this change
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 nix-review --run "nix-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.
Notify maintainers

cc @

@ofborg ofborg bot requested a review from vbgl November 26, 2019 12:51
@marsam
Copy link
Contributor Author

marsam commented Nov 27, 2019

@GrahamcOfBorg build fstar

@ofborg ofborg bot requested a review from volth November 27, 2019 12:54
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/explicit-runhook-in-derivation/4614/34

@vyorkin
Copy link
Member

vyorkin commented Dec 5, 2019 •
edited

Do you want to add dune-configurator as well or would it be better to add it in a separate PR?

Zimmi48
Zimmi48 reviewed Dec 14, 2019
View reviewed changes
Copy link
Member

@Zimmi48 Zimmi48 left a comment

Choose a reason for hiding this comment

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

Not having Dune 2.0.0 yet in nixpkgs is an inconvenience. Can you please explain what are the things that are blocking this PR or what additional work you think is required?

I am not sure I understand the first commit, adding Dune 2.0, but keeping Dune 1.11 without exposing it. Why would we want to keep Dune 1.11 in nixpkgs?

If this is because Dune 2.0 is not compatible with OCaml < 4.06, note that we could go back to the situation preceding #57953 where Dune was out of ocamlPackages. This was changed because Dune is both a tool and a library, but we could try to find another solution, e.g. making it available as a library only for ocamlPackages sets starting with 4.06 and as a tool independent of ocamlPackages for any OCaml version.

I suppose it is not because some OCaml packages have not been migrated yet from jbuilder to Dune, otherwise you wouldn't be updating a bunch of packages in this PR as well. BTW, for packages that still haven't migrated to Dune in their latest version, maybe that says something about them (not) being maintained, and we could simply choose to drop them, or at least mark them as broken and not care much...

pkgs/development/ocaml-modules/stdint/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/ocaml/cppo/default.nix Outdated Show resolved Hide resolved
@marsam marsam marked this pull request as ready for review December 18, 2019 15:37
@marsam
Copy link
Contributor Author

marsam commented Dec 18, 2019

Why would we want to keep Dune 1.11 in nixpkgs?

dune 2 is a major version bump, a there are multiple packages which still haven't been migrated, for instance, janestreet packages.

I tried to set dune 2 as default, but I encountered various errors, hence only added dune_2 only as another attribute.

Only the last three commits are relevant; @vbgl should I cut the other commits into a separated PR?

@Zimmi48
Copy link
Member

Zimmi48 commented Dec 18, 2019

dune 2 is a major version bump, a there are multiple packages which still haven't been migrated, for instance, janestreet packages.

In theory, Dune 2 shouldn't break Dune projects which set the Dune language version to a lower version than 2.0. However, it does break the projects that have not migrated to Dune from jbuilder. Did you find examples beyond this? If yes, it could be interesting to look into them more closely and possibly report the issues to Dune developers.

In any case, this PR LGTM (in full, including the version bumps) and I'm in favor of merging it as-is now (without splitting).

@Zimmi48
Copy link
Member

Zimmi48 commented Dec 20, 2019

@marsam I'd like to encourage you to go ahead and merge this without waiting further.

@Zimmi48
Copy link
Member

Zimmi48 commented Dec 23, 2019

@marsam or @vbgl: please merge this now.

BTW: PR title should be edited to reflect the change (dune_2: init at 2.1.0).

@marsam marsam changed the title dune: init at 2.0.0 dune: init at 2.1.0 Dec 23, 2019
@marsam
Copy link
Contributor Author

marsam commented Dec 23, 2019

@Zimmi48 sorry for the delay, I was waiting for another review; merging as-is because it shouldn't introduce any major change. @vbgl I hope you don't mind

@marsam marsam merged commit 8d3199b into NixOS:master Dec 23, 2019
@marsam marsam deleted the update-dune branch December 23, 2019 14:19
@Zimmi48
Copy link
Member

Zimmi48 commented Dec 23, 2019

Thanks!

@Shados
Copy link
Member

Shados commented Jan 4, 2020

@marsam This PR included an update for cppo from 1.6.5 to 1.6.6. This broke building cppo with ocaml 4.02, which is still needed for some things.

@marsam
Copy link
Contributor Author

marsam commented Jan 4, 2020

@Shados sorry about that, reverted in #76942

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zimmi48 Zimmi48 Zimmi48 left review comments

@vbgl vbgl Awaiting requested review from vbgl

Assignees
No one assigned
Labels
6.topic: ocaml 8.has: clean-up 8.has: package (new) 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@marsam @nixos-discourse @vyorkin @Zimmi48 @Shados @vbgl