-
Notifications
You must be signed in to change notification settings - Fork 6
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
download miniforge instead of mambaforge #60
Conversation
WalkthroughThe The script maintains its existing logic for verifying the presence of required files and variables, with distinct handling for macOS and Linux installations. For macOS, the script includes commands to remove any existing installations and adjust permissions, while the Linux section focuses on creating the necessary directory structure. The conditional installation of Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
install-and-set-up-conda.sh (4)
Line range hint
3-7
: Update script documentation to reflect Miniforge usageThe comments still reference "mambaforge" but should be updated to "miniforge" for consistency with the changes being made.
-# - Installs mambaforge to ${HOME}/mambaforge. Version is determined by common.sh, +# - Installs miniforge to ${HOME}/miniforge. Version is determined by common.sh,
53-58
: Update local filename for consistencyWhile the URL has been updated to use Miniforge, the local filename is still "mambaforge.sh". This should be updated for consistency.
MINIFORGE_URL="https://github.com/conda-forge/miniforge/releases/download/${MAMBAFORGE_VER}/Miniforge3-${MAMBAFORGE_VER}-${OS}-${ARCH}.sh" -echo Download ${MINIFORGE_URL} -curl -L ${MINIFORGE_URL} > mambaforge.sh -head mambaforge.sh -bash mambaforge.sh -b -p "${MAMBAFORGE_INSTALLATION_DIR}" +echo "Download ${MINIFORGE_URL}" +curl -L ${MINIFORGE_URL} > miniforge.sh +head miniforge.sh +bash miniforge.sh -b -p "${MAMBAFORGE_INSTALLATION_DIR}"
Line range hint
82-93
: Complete the transition by updating installation pathsThe script still uses
MAMBAFORGE_INSTALLATION_DIR
for various operations. For a complete transition, consider updating:
- Installation directory variable and references
- Directory creation commands
- Environment setup paths
This should be coordinated with changes in common.sh and other dependent repositories. Consider:
- Creating a migration plan
- Testing the changes in a staging environment
- Updating all dependent scripts and documentation
Would you like assistance in creating a comprehensive migration plan?
Variables need to be updated to reflect Miniforge transition
The review comment is correct. Based on the file contents, there's a clear inconsistency where the code uses Miniforge URL and installation but still refers to variables with "mambaforge" naming. Specifically:
- In
common.sh
: VariablesMAMBAFORGE_VER
andMAMBAFORGE_INSTALLATION_DIR
need to be renamed- In
configure-conda.sh
: References toMAMBAFORGE_INSTALLATION_DIR
- In
install-and-set-up-conda.sh
: References to both variables and comments mentioning mambaforgeThe changes needed:
- Rename
MAMBAFORGE_VER
toMINIFORGE_VER
- Rename
MAMBAFORGE_INSTALLATION_DIR
toMINIFORGE_INSTALLATION_DIR
- Update all references in the three affected files
- Update comments to reflect Miniforge instead of Mambaforge
🔗 Analysis chain
Line range hint
19-26
: Broader changes needed for complete transitionThe script still relies on variables like
MAMBAFORGE_VER
andMAMBAFORGE_INSTALLATION_DIR
from common.sh. These should be renamed to maintain consistency with the Miniforge transition.Let's verify other potential dependencies:
Let me check the actual content of these files to understand the full context of the variable usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining mambaforge references in the repository rg -i "mambaforge" --type-add 'script:*.{sh,bash}' --type scriptLength of output: 343
Script:
#!/bin/bash # Check content of relevant files echo "=== common.sh ===" cat common.sh echo -e "\n=== configure-conda.sh ===" cat configure-conda.sh echo -e "\n=== install-and-set-up-conda.sh ===" cat install-and-set-up-conda.shLength of output: 5743
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
install-and-set-up-conda.sh
(1 hunks)
🔇 Additional comments (1)
install-and-set-up-conda.sh (1)
Line range hint 1-99
: Overall transition strategy needed
While this PR initiates the transition from Mambaforge to Miniforge, several areas still need attention:
- Script documentation
- Variable names in common.sh
- Installation directory references
- Local file naming
- Dependencies in other repositories
Consider creating a comprehensive migration plan to coordinate these changes across all affected repositories.
Let's check for potential impacts:
#!/bin/bash
# Search for files that might need updates
echo "Files containing installation dir references:"
rg -l "MAMBAFORGE_INSTALLATION_DIR"
echo -e "\nFiles containing version references:"
rg -l "MAMBAFORGE_VER"
Thanks! |
I hope this will get us going. Ideally we would need to change all occurrences of
MAMBAFORGE
withMINIFORGE
. But this is a larger surgery in multiple repos I think.