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

ocamlPackages.lwt_ppx: use independent source from lwt #77182

Merged
merged 1 commit into from Jan 8, 2020

Conversation

@anmonteiro
Copy link
Member

@anmonteiro anmonteiro commented Jan 7, 2020

Motivation for this change

I also added a comment in the packaging code itself, but basically Lwt does sometimes release versions without releasing lwt_ppx. In those versions, a broken lwt_ppx package will be in these lwt release tarballs without dev-time helper dependendency declarations stripped (bisect_ppx), causing an error building the lwt_ppx package.

Additionally, the versioning scheme for lwt_ppx is different than Lwt's own version; this diff also fixes that.

Tagging Lwt maintainer @aantron for confirmation.

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.
Notify maintainers

cc @vbgl @Zimmi48 (git history)

anmonteiro added a commit to anmonteiro/nix-overlays that referenced this pull request Jan 7, 2020
while NixOS/nixpkgs#77182 is pending review
@aantron
Copy link

@aantron aantron commented Jan 7, 2020

Tagging Lwt maintainer @aantron for confirmation.

The above is indeed the case. lwt_ppx and lwt are released from a monorepo, but have separate release cycles and separate version numbers. For example, at the moment, the absolute latest lwt is 5.1.1, and lwt_ppx is at 2.0.0. A release of one package from the monorepo does not necessarily imply a release of the other (nor of the third package in the monorepo, lwt_react); however, the release tarball will contain the code of both, because it is a snapshot of a monorepo. Dune (the OCaml build system) "knows" how to build only the right package from this tarball, and ignore the code of others. While the unreleased packages in the tarball are not generally outright broken, they are often in a development state, with development dependencies still baked in, so it is not correct to assume that they can be built in release mode from the tarball, and it's not safe to treat each tarball as a release of all the packages.

@Zimmi48
Zimmi48 approved these changes Jan 7, 2020
Copy link
Member

@Zimmi48 Zimmi48 left a comment

Thank you for finding out and fixing this!

To me this shows one of the limitation of using monorepos (which can be a great choice anyway for many other reasons). In particular, it is a bit frustrating that the tarball URL must hardcode another version number, different from the one of lwt_ppx. @aantron As the package maintainer, you are fully entitled to proceed the way you wish, but what about using tags which contain the name of the package being released (such as lwt-4.4.0 and lwt_ppx-1.2.4)? Given that two tags can point to the same commit, it wouldn't even prevent releasing two packages at once, but would make updating to the new version of lwt_ppx straightforward. Finally, regarding the fact that the archive contains sources of unrelated packages, wouldn't generating a dune-release tarball allow shipping only the relevant code (I'm not sure as I've never used dune-release myself)?

@aantron
Copy link

@aantron aantron commented Jan 8, 2020

The reason Lwt is a monorepo is because it was factored into several packages a couple years ago. Most of them were then moved to separate repos or deleted, but we chose to leave some of the more "core" packages in the repo. It's not necessarily a decision to favor a monorepo specifically.

Having independent tags for the two helper packages would introduce enough noise in the releases and tags list that I'd rather move them out to separate repos instead. As for shipping sources separately, I'd also rather separate the repos than use any tool for that. That is another thing that could go wrong during the release process, whereas shipping the repo snapshot guarantees that the code that is delivered is the same code that was tested during development. The helper packages are trivial in size compared to Lwt. On the other hand, for someone installing a helper package, Lwt will already be in cache, because the helper packages depend on Lwt. So, I don't (yet) see a benefit from doing this - it's only an extra complication.

What is the process of updating to new version of lwt_ppx? How do you find out that there is a new version? What do you do then? What do you edit, what does the diff look like?

@Zimmi48
Copy link
Member

@Zimmi48 Zimmi48 commented Jan 8, 2020

@aantron Again, this was just a soft suggestion. Your current workflow is fine, and further splitting the repo would probably hinder your processes, so I wouldn't want to push for that. Lots of package definitions, including lwt, are defined quite simply: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/ocaml-modules/lwt/4.x.nix, with a URL for the source that is built by interpolating a base string with the version number. Therefore, updates can be as simple as bumping the version number and fixing the sha to a new value (which is computed automatically and is there for reproducibility). Example for another OCaml package: d3ff8e6. In some cases, it may additionally require other fixes, such as changing the list of dependencies. Example of the last lwt update: 6cffb50.

Some parts of nixpkgs even use a bot that automates the version bump process when it is straightforward (see automated PR list: https://github.com/NixOS/nixpkgs/pulls/r-ryantm) and that is combined with automated tests, but it has never been used so far for OCaml packages (thus updates are always manually generated when a maintainer notices a package is outdated and it would be useful to bump the version number).

In the case of lwt_ppx, this just means that bumping the version number will always require more manual work, and more manual review, but this is perfectly fine.

@vbgl vbgl merged commit ccce14e into NixOS:master Jan 8, 2020
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@aantron
Copy link

@aantron aantron commented Jan 8, 2020

I hope this is a vanishing issue. It was actually my preference to also take lwt_ppx and lwt_react out of the repo :) lwt_ppx should gradually become less important with the new let+ syntax and/or eventual support for algebraic effects, and then we will probably split it off. As for lwt_react, I'll probably just argue again to remove it, on the basis of how few issues it generates.

Thanks for the links!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.