Skip to content

chore(nix): make it faster to get melange#6347

Merged
rgrinberg merged 3 commits intoocaml:mainfrom
anmonteiro:anmonteiro/melange-without-flambda
Nov 15, 2022
Merged

chore(nix): make it faster to get melange#6347
rgrinberg merged 3 commits intoocaml:mainfrom
anmonteiro:anmonteiro/melange-without-flambda

Conversation

@anmonteiro
Copy link
Copy Markdown
Collaborator

  • nix-overlays enables flambda by default
  • depending on the melange flake forces dune developers to build an extra flambda compiler to work on Dune
  • this change uses the upstream OCaml from nixpkgs which should always be cached in cache.nixos.org

@anmonteiro anmonteiro requested a review from Alizter October 28, 2022 00:52
flake.nix Outdated
ocaml-ng = super.ocaml-ng // {
ocamlPackages_4_14 =
super.ocaml-ng.ocamlPackages_4_14.overrideScope' (oself: osuper: {
dune_3 = osuper.dune_3.overrideAttrs (o: {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

should only be needed until NixOS/nixpkgs#196899 is merged.

@anmonteiro anmonteiro force-pushed the anmonteiro/melange-without-flambda branch from dd365da to 0b19f46 Compare October 28, 2022 00:55
Copy link
Copy Markdown
Collaborator

@Alizter Alizter left a comment

Choose a reason for hiding this comment

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

Thanks!

@Alizter Alizter requested a review from rgrinberg October 29, 2022 00:43
@anmonteiro anmonteiro added the melange Melange rules and generator label Oct 29, 2022
@rgrinberg
Copy link
Copy Markdown
Member

@anmonteiro instead of waiting for upstream, could you add an overlay here?

flake.nix Outdated
flake = false;
};
melange.url = "github:melange-re/melange";
melange.inputs.nixpkgs.follows = "nixpkgs";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

instead of reusing our own nixpkgs, would it be possible to just insert an overlay into melange's nixpkgs?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If I understood you correctly, I don't think that's a good idea. Melange's nixpkgs are my nix overlays, which is what's causing the issue of having 2 OCaml compilers in the first place (clambda and flambda).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right but we can override your overlay? We don't need a fast melange since we only run it in trivial test projects.

Copy link
Copy Markdown
Collaborator Author

@anmonteiro anmonteiro Nov 15, 2022

Choose a reason for hiding this comment

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

You're right, the line is not strictly necessary. It avoids pulling in one more tarball though. I'll defer to your decision.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess if our goal is to speed up the build of melange this is ideal.

I'm just worried that if you decide to modify or overlay your nixpkgs somehow, melange won't build in our dev shell.

Anyway it's enough to keep an eye out for this for now.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Alright, I removed the follows line. It doesn't impact the build: we just download an extra tarball of nixpkgs.

@anmonteiro
Copy link
Copy Markdown
Collaborator Author

@anmonteiro instead of waiting for upstream, could you add an overlay here?

I like that suggestion. will do

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/melange-without-flambda branch from 48c711c to 18ec685 Compare November 15, 2022 18:20
@anmonteiro
Copy link
Copy Markdown
Collaborator Author

anmonteiro commented Nov 15, 2022

Switched this to use a melange-provided overlay (added in melange-re/melange#430).

flake.nix Outdated
pname = "melange-compiler-libs";
version = "0.0.1-414";
src = builtins.fetchurl {
url = "https://github.com/melange-re/melange-compiler-libs/releases/download/${version}/melange-compiler-libs-${version}.tbz";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be possible to add melange-compiler-libs as a flake input instead?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done in 76eab97

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
Copy link
Copy Markdown
Collaborator Author

Diff looks a lot better with https://github.com/ocaml/dune/pull/6347/files?w=1

@rgrinberg rgrinberg merged commit abb4a38 into ocaml:main Nov 15, 2022
@anmonteiro anmonteiro deleted the anmonteiro/melange-without-flambda branch November 15, 2022 20:57
jchavarri added a commit to jchavarri/dune that referenced this pull request Nov 16, 2022
* main:
  test(melange): demonstrate error in melange compilation trying to build @ALL (ocaml#6485)
  chore(nix): make it faster to get melange (ocaml#6347)
  refactor: status db style tweaks (ocaml#6478)
  fix: improve error message for status db (ocaml#6479)
  refactor: remove unused [flags] parameter (ocaml#6480)
  refactor(ctypes): remove pesky aliases (ocaml#6482)
  chore: tweak `hacking.rst` following `dune.exe` move to _boot (ocaml#6484)
  feature(coq): automatic detection of native
  chore(coq): bump Coq lang to 0.7
  test: disable formatting for a single dune file (ocaml#6465)
  refactor: clean up module compilation (ocaml#6461)
  doc: add button to copy code blocks in Dune manual (ocaml#6428)
  refactor: deforest a set conversion (ocaml#6473)
  refactor: remove temporary map used for sorting (ocaml#6472)
  fix(melange): handle include_subdirs unqualified (ocaml#6475)
jchavarri added a commit to jchavarri/dune that referenced this pull request Nov 16, 2022
* main:
  test(melange): demonstrate error in melange compilation trying to build @ALL (ocaml#6485)
  chore(nix): make it faster to get melange (ocaml#6347)
  refactor: status db style tweaks (ocaml#6478)
  fix: improve error message for status db (ocaml#6479)
  refactor: remove unused [flags] parameter (ocaml#6480)
  refactor(ctypes): remove pesky aliases (ocaml#6482)
  chore: tweak `hacking.rst` following `dune.exe` move to _boot (ocaml#6484)
  feature(coq): automatic detection of native
  chore(coq): bump Coq lang to 0.7
  test: disable formatting for a single dune file (ocaml#6465)
  refactor: clean up module compilation (ocaml#6461)
  doc: add button to copy code blocks in Dune manual (ocaml#6428)
  refactor: deforest a set conversion (ocaml#6473)
  refactor: remove temporary map used for sorting (ocaml#6472)
  fix(melange): handle include_subdirs unqualified (ocaml#6475)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

melange Melange rules and generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants