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

[build] Add script for Deeploy environment variables and Makefile info #5

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

Conversation

viv-eth
Copy link

@viv-eth viv-eth commented Oct 3, 2024

This PR introduces a new script (deeploy.sh) that sets up essential environment variables for the Deeploy project. The Makefile has been updated to reference this script, providing users an easy way to source environment variables directly from deeploy.sh after setting up the PULP SDK.

Changes:

  1. Added deeploy.sh script for defining paths to critical tools (LLVM, PULP SDK, QEMU, etc.)
  2. Updated Makefile to include instructions on sourcing the deeploy.sh script for proper environment configuration

This update simplifies the setup process by centralizing environment variables, reducing the need for manual configuration.

@viv-eth viv-eth marked this pull request as ready for review October 3, 2024 14:00
Copy link
Collaborator

@Scheremo Scheremo left a comment

Choose a reason for hiding this comment

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

Hey Viviane, thanks for your contribution.
I personally don't like sourcing bash scripts to set up the environment. Offering as an alternative is probably okay, though. In any case, please use the tool-specific path variables rather than DEEPLOY_INSTALL_DIR

Comment on lines +1 to +4
export PULP_SDK_HOME=${DEEPLOY_INSTALL_DIR}/pulp-sdk
export LLVM_INSTALL_DIR=${DEEPLOY_INSTALL_DIR}/llvm
export PULP_RISCV_GCC_TOOLCHAIN=
export MEMPOOL_HOME=${DEEPLOY_INSTALL_DIR}/mempool
Copy link
Collaborator

Choose a reason for hiding this comment

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

You shouldn't just set all these paths relative to DEEPLOY_INSTALL_DIR. The Makefile is set up such that previous installations may be reused, and each tool gets its own DIR variable.

@@ -72,12 +72,15 @@ echo-bash:
@echo "TL/DR: add these lines to run ~/.bashrc"
@echo "export PULP_SDK_HOME=${PULP_SDK_INSTALL_DIR}"
@echo "export LLVM_INSTALL_DIR=${LLVM_INSTALL_DIR}"
@echo "export PULP_RISCV_GCC_TOOLCHAIN=/PULP_SDK_IS_A_MESS"
@echo "export PULP_RISCV_GCC_TOOLCHAIN="
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not completely remove this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The pulp-sdk needs the environment variable set afaik but @Scheremo could confirm.
I would prefer if the contents of the variable stay untouched 😏

@Victor-Jung
Copy link
Member

Ciao Viviane, I'd be happy to merge this PR once the feedback from Moritz and Philip is applied and the bash script is moved to the scripts folder.

@Victor-Jung Victor-Jung self-requested a review October 28, 2024 11:52
@Xeratec
Copy link
Collaborator

Xeratec commented Nov 24, 2024

@viv-eth What is the status of that PR? Should I implement the requested changes?

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.

5 participants