-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
Ruby 2.7.2 & Windows support #41
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
btw @timsnyder once I get this working I am happy to rebase on your commits so you get the credit. |
This is ready to go! |
@conda-forge/core I think this is ready to go! |
Let's wait for @scopatz, @sodre, @jimmynguyc before asking core to merge. |
if not exist %PREFIX%\etc\conda\%%F.d mkdir %PREFIX%\etc\conda\%%F.d | ||
copy %RECIPE_DIR%\%%F.bat %PREFIX%\etc\conda\%%F.d\%PKG_NAME%_%%F.bat | ||
:: Copy unix shell activation scripts, needed by Windows Bash users | ||
copy %RECIPE_DIR%\%%F.sh %PREFIX%\etc\conda\%%F.d\%PKG_NAME%_%%F.sh |
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.
These copies and mkdirs need exit conditions
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.
In that case we shoudl fix the docs: https://docs.conda.io/projects/conda-build/en/latest/resources/activate-scripts.html
I don't really think that there is a high likelyhood it would fail because e.g. out of space for copying these small files.
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.
Yeah agreed, the docs should be updated
Couple of small comments! Thanks wolf! |
…a-build 3.19.2, conda-smithy 3.7.10, and conda-forge-pinning 2020.09.19.20.20.14
fixed all comments |
@scopatz wanna merge? I could use the windows build in another feedstock |
recipe/deactivate.sh
Outdated
# Windows case | ||
if [[ $dir != "${CONDA_PREFIX}/Library/share/rubygems/bin" ]]; then | ||
PATH="${PATH:+$PATH:}$dir" | ||
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.
The logic doesn't seem right here. Won't this add each $dir
twice to the path if it isn't one of the disallowed paths and once if it is disallowed?
Yeah, I'll merge as soon as the deactivate logic is worked out 😉 |
Hi @scopatz @wolfv, /cc @traversaro |
@scopatz this should be good to go now, for real :) |
Checklist
0
(if the version changed)conda-smithy
(Use the phrase@conda-forge-admin, please rerender
in a comment in this PR for automated rerendering)