-
Notifications
You must be signed in to change notification settings - Fork 630
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
Use conda + Update initial setup docs #1163
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems mostly fine.
I would reduce the amount of stuff the machine-launch-script.sh has to do. If we assume Conda is installed already, than the script does not need SUDO, and it can be renamed to conda-env-setup.sh
or something (which is much more descriptive than "machine launch").
We should generally avoid giant multi-purpose scripts with a bunch of flags that leave effects on the user's environment. This is really only acceptable for the F1 case, where the instances are "disposable".
I've sketched out where the conda activate calls should go. Once that is settled I'll update the docs to reflect that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks close, but I think with some changes we can support the no-conda use case too.
scripts/conda-activate-env.sh
Outdated
conda activate firesim | ||
else | ||
conda activate chipyard | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would there be a firesim
env? Why not just tell the user to always conda activate chiypard
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FireSim by default calls it's env firesim
. So if we want to support Chipyard in the FireSim setup (on AWS F1) then we should switch between them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs say
Warning:: When running on an Amazon Web Services EC2 FPGA-development instance (for FireSim), FireSim includes a similar machine setup script that will install all of the aforementioned dependencies (and some additional ones). Skip this section.
This instructs users running Chipyard in FireSim to skip running this script, since I believe the fsim docs should contain instructions for activating the firesim env. In that case, this script doesn't need to exist, and the docs can just say conda activate chipyard
For my own understanding, I believe the flow is:
- If FireSim, follow firesim docs, then skip to 5
- install conda
- create chipyard conda env
- activate conda env
- init submodules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was re-worded in the past. Better now?
Cancelling CI since there needs to be more work done to make that work. |
c2c5cc5
to
8007c6a
Compare
This PR is good to merge. The errors for SPI flash/read are because we need to bump Verilator to 4.226 and the CVA6 error is because there is a segfault when CVA6 is used with DRAMSim2 in the new verilator (this is solved if you use magic memory) |
CVA6 with VCS + DRAMSIM is still fine? |
I haven't had the chance to verify this on this PR but pre-Conda I do remember this working. I can confirm that VCS + CVA6 + DRAMSim2 works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo a few fixes in comments.
I've gone ahead and fixed this by removing a |
Co-authored-by: Sagar Karandikar <sagark@eecs.berkeley.edu>
This PR removes the
*-req.sh
scripts in favor of using Conda (same system that FireSim uses to manage system packages). I've also changed the documentation to be a bit clearer on how to run things in a AWS EC2 context and in the normal scenario. Notable changes are below:conda-requirements*.yaml
files. Additionally, lock files*.conda-lock.yml
files are provided to ensure a consistent set of packages.setup.sh
file that automates the setup steps including setting up a conda environment, submodule cloning, and building extra toolchain collateral (i.e. Spike, PK, etc)buildroot
/fedora
build with it). Also, use QEMU built by conda in theucb-bar
channel.ucb-bar
conda channel (riscv-tools
andesp-tools
are both provided)self-hosted
runners for test runs. Needed since GH-A cache action now doesn't support caching between Docker containers and self-hosted runners.Related PRs / Issues:
Type of change:
Impact:
Contributor Checklist:
main
as the base branch?changelog:<topic>
label?changelog:
label?Please Backport
?