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

chore(nix): support build for experimental feature #10845

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

maiste
Copy link
Collaborator

@maiste maiste commented Aug 23, 2024

Following the merge of #10724, I have written this PR so we are able to build the executable using Nix Flake. I'm far from being a Nix expert, so if someone with a better expertise is able to reduce the amount of code, it would be happy to change the code 😄

EDIT: Replace my solution with @gridbugs' one, as it is simpler and more elegant one. It consists in adding a function to the package default that could take some flags.

@rgrinberg rgrinberg requested a review from emillon August 24, 2024 06:11
@maiste maiste force-pushed the chore/nix branch 2 times, most recently from 1412a86 to 2aa669b Compare August 27, 2024 15:50
@gridbugs
Copy link
Collaborator

Heads up that I pushed a commit that fixes the failing nix test.

@gridbugs
Copy link
Collaborator

I pushed another change to remove the duplication of the experimental flags.

@emillon
Copy link
Collaborator

emillon commented Sep 2, 2024

I think it's simpler and more idiomatic to use overrideAttrs to update the configureFlags of an existing derivation, I'll try something

@emillon emillon force-pushed the chore/nix branch 2 times, most recently from 22c38a4 to 0eb22e6 Compare September 2, 2024 15:04
@emillon
Copy link
Collaborator

emillon commented Sep 2, 2024

how does it look @maiste ? I did a quick test and it seems to work but I'm not super familiar with the experimental flags so let me know if the resulting binaries work as you expect

@maiste
Copy link
Collaborator Author

maiste commented Sep 2, 2024

On @gridbugs and my computer, the produced binary works as expected:

  • No need to add some flags to get the compiler installed (no error about Ocaml when doing the dune pkg lock command)
  • It displays the packages installed nicely when running dune build

EDIT: I didn't realize I didn't update the branch before trying. I retried with the musl version, and it worked! 👌

@gridbugs
Copy link
Collaborator

gridbugs commented Sep 3, 2024

Nice work @emillon, I didn't know about overrideAttrs. I gave this a test on both macos-aarch64 and linux-x86_64 and both binaries have the features I expected.

@maiste
Copy link
Collaborator Author

maiste commented Sep 3, 2024

I have rebased and cleaned the history. If the tests pass, I think it's ok to merge it 👍

@emillon
Copy link
Collaborator

emillon commented Sep 3, 2024

Nice work @emillon, I didn't know about overrideAttrs

The part that made it click for me is this comment - derivations are mkDerivation calls and overrideAttrs changes the input of this function.

{ stdenv, bar, baz }: # this part gets overriden by `override`
stdenv.mkDerivation { # This part gets overriden by overrideAttrs
  pname = "test";
  version = "0.0.1";
  buildInputs = [bar baz];
  phases = ["installPhase"];
  installPhase = "touch $out";
}

@maiste
Copy link
Collaborator Author

maiste commented Sep 4, 2024

@gridbugs @emillon, I don't have the merge right: can I gently ask one of you to do the merge? :)

Use `overrideAttrs` to change the behaviour and export
new target `dune-experimental` and `dune-static-experimental`

Co-authored-by: Etienne Millon <me@emillon.org>

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Etienne Millon <me@emillon.org>
@gridbugs gridbugs merged commit 864dfd7 into ocaml:main Sep 4, 2024
26 of 27 checks passed
@maiste maiste deleted the chore/nix branch September 4, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants