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

nixpkgs_flake_package() supplies --experimental-features 'nix-command flakes' in case someone's local nix.conf hasn't enabled them. #465

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

rickvanprim
Copy link
Contributor

@layus does this seem reasonable? I'm not aware of any downsides for unconditionally passing these flags.

@rickvanprim rickvanprim requested a review from benradf as a code owner December 11, 2023 18:14
Copy link
Collaborator

@layus layus left a comment

Choose a reason for hiding this comment

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

I think you need --extra-experimental-features to avoid wiping the features already present in the user config. Otherwise, yes, this is a very good idea 💯

core/nixpkgs.bzl Outdated
@@ -790,7 +790,7 @@ def _nixpkgs_flake_package_impl(repository_ctx):
extra_msg = "See: https://nixos.org/nix/",
)

_nixpkgs_build_and_symlink(repository_ctx, [nix_path, "build"], expr_args, build_file_content)
_nixpkgs_build_and_symlink(repository_ctx, [nix_path, "--experimental-features", "nix-command flakes", "build"], expr_args, build_file_content)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
_nixpkgs_build_and_symlink(repository_ctx, [nix_path, "--experimental-features", "nix-command flakes", "build"], expr_args, build_file_content)
_nixpkgs_build_and_symlink(repository_ctx, [nix_path, "--extra-experimental-features", "nix-command flakes", "build"], expr_args, build_file_content)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Done 🙂

…and flakes'` in case someone's local `nix.conf` hasn't enabled them.
@rickvanprim rickvanprim force-pushed the flake_experimental_features branch from 3962b20 to 7a832ba Compare December 11, 2023 21:57
@rickvanprim
Copy link
Contributor Author

rickvanprim commented Dec 11, 2023

One more thought I have while I'm in here, is_supported_platform() is currently:

def is_supported_platform(repository_ctx):
    return repository_ctx.which("nix-build") != None

Should this switch to checking nix at some point? And/or should nixkpkgs_flake_package() check nix and the other rules continue checking nix-build?

@aherrmann
Copy link
Member

The existence of nix-build is just used as a proxy for whether Nix is installed. With modern versions of Nix it could equivalently check for nix, I think. But, I don't think we need to conflate these in this PR.

@benradf benradf requested a review from layus December 13, 2023 16:38
@layus layus added the merge-queue merge on green CI label Dec 14, 2023
@layus layus merged commit 244ae50 into tweag:master Dec 14, 2023
13 of 15 checks passed
@mergify mergify bot removed the merge-queue merge on green CI label Dec 14, 2023
@rickvanprim rickvanprim deleted the flake_experimental_features branch December 14, 2023 20:27
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.

4 participants