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

Feat/nix shell #48

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Feat/nix shell #48

wants to merge 5 commits into from

Conversation

ESCristiano
Copy link
Member

@ESCristiano ESCristiano commented Jun 18, 2023

PR Description

Add a nix-shell environment to contemplate all essential bao-project dependencies, allowing developers to effortlessly work across all bao-project repositories.

The user can utilize the bao-project nix-shell in three ways:

  1. Run a standalone command within the bao-project nix-shell environment;
make -C ci/nix nix-shell-run cmd="your command"
  1. Run a Ci Rule within the bao-project nix-shell environment;

For example, to run format check:

make -C ci/nix format-check
  1. Open an interactive bao-project nix-shell environment.
make -C ci/nix nix-shell

Supported Bao-Project Repositories

The ultimate goal is to provide support for all the bao-projects repositories. Currently, the nix-shell only handles dependencies for bao-docs. Support for the remaining repositories will be incorporated in the future.

Type of change

  • feat: introduces a new functionality
    • Logical unit: nix/shell.nix
    • Logical unit: nix/Makefile

Checklist:

  • The changes generate no new warnings when building the project. If so, I have justified above.
  • I have run the CI checkers before submitting the PR to avoid unnecessary runs of the workflow.

# bao-project repositories.
# To open an interactive nix-shell with the bao-project environment:
# make nix-shell
# To run a standalone commnand inside the nix-shell use:
Copy link
Member

Choose a reason for hiding this comment

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

Fix typo to command

Copy link
Member Author

Choose a reason for hiding this comment

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

I will fix the typo.

# make nix-shell
# To run a standalone commnand inside the nix-shell use:
# make nix-shell-run cmd="command-to-run"
# @param The command to run inside the nix-shell.
Copy link
Member

Choose a reason for hiding this comment

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

@param command-to-run The command to run inside the nix-shell.
I understood the description but it is in order to obey the doxygen comment style @param <variable_name> <description>

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand your comment, however, I just followed the description used in other rules on the makefile.

Copy link
Member

@josecm josecm left a comment

Choose a reason for hiding this comment

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

We should also modify the README introduction to highlight we have this build environment two.

ci.mk Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@josecm
Copy link
Member

josecm commented Jul 27, 2023

@ESCristiano I've updated the README introduction to mention the nix-shell facility.

@ESCristiano
Copy link
Member Author

@josecm I've incorporated your suggestion and now nix can be used in a similar way as docker.

I've also updated the PR description to reflect these changes.

@nix-shell $(nix_dir)/shell.nix

nix-shell-run:
@nix-shell $(nix_dir)/shell.nix --run "$(cmd)"
Copy link
Member

Choose a reason for hiding this comment

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

Although we have no documentation on this, I typically make variables that can be passed by the user upper case, so:

Suggested change
@nix-shell $(nix_dir)/shell.nix --run "$(cmd)"
@nix-shell $(nix_dir)/shell.nix --run "$(CMD)"

Comment on lines +29 to +33
nix-shell:
@nix-shell $(nix_dir)/shell.nix

nix-shell-run:
@nix-shell $(nix_dir)/shell.nix --run "$(cmd)"
Copy link
Member

Choose a reason for hiding this comment

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

I'd say first we could drop the nixprefixes on the make targets because this makefile is standalone, specifying nix shell is redundant as this is a the nix makefile.

Also, it would be possible to have a single target and recipe that would behave differently if CMD was specified by the user. In sum, something like:

Suggested change
nix-shell:
@nix-shell $(nix_dir)/shell.nix
nix-shell-run:
@nix-shell $(nix_dir)/shell.nix --run "$(cmd)"
ifneq ($(CMD),)
CMD:= --run $(CMD)
endif
shell:
@nix-shell $(nix_dir)/shell.nix $(CMD)

# SPDX-License-Identifier: Apache-2.0
# Copyright (c) Bao Project and Contributors. All rights reserved

root_dir?=$(realpath ..)
Copy link
Member

Choose a reason for hiding this comment

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

I'd say the root dir by default should be:

Suggested change
root_dir?=$(realpath ..)
root_dir?=$(realpath ../../)

Which, by our convention on how the ci repo should be included in the project's repos, should be the repo top-level directory and where all commands by default should run. And then, in the recipes:

nix-shell:
	@cd $(root_dir) && nix-shell $(nix_dir)/shell.nix

nix-shell-run:
	@cd $(root_dir) && nix-shell $(nix_dir)/shell.nix --run "$(cmd)"

Copy link
Member

Choose a reason for hiding this comment

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

Since we have the nix only makefile, I think the modifications for this file should be removed.

Comment on lines +36 to +40
non_build_targets+=nix-shell nix-shell-run

define nix-shell-run
cmd:=$1
endef
Copy link
Member

@josecm josecm Oct 13, 2023

Choose a reason for hiding this comment

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

I don't think these are actually needed.

nix-shell-run:
@nix-shell $(nix_dir)/shell.nix --run "$(cmd)"

.PHONY: all
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.PHONY: all
.PHONY: all nix-shell nix-shell-run

@josecm josecm self-assigned this Oct 26, 2023
@josecm josecm force-pushed the main branch 2 times, most recently from eede022 to f0fba61 Compare July 16, 2024 10:44
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