-
Notifications
You must be signed in to change notification settings - Fork 70
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
Fixes #166: Error on miniconda-version *not* specified instead of when it *is* specified. #189
Conversation
Raise an error if on non-x64 architectures, `miniconda-version` is *not* specified, instead of when it *is* specified.
a2e1811
to
8cc7c0e
Compare
Thanks! Do we need a test for this or is it enough with what we have so far? |
I thought I'd add that this does not fix the more general problem of old conda builds (many of which are 32-bit) being unsupported - ongoing attempts to address that issue are in #168. However, some 32-bit builds, namely Windows, have recent relases:
So this fix restores the ability to use them, at least. |
@jaimergp I think tests would show that 32 bit support is still not quite there, but I don't think that fact should block this obvious bugfix from coming in. |
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 but I'd have @bollwyvl have a look too!
Yep, the fix looks good. As it was a bug, we probably do need a test, which can then also be used for documentation purposes in the README. Also it probably needs to have the build re-run to update the transpiled assets |
Co-authored-by: ktbarrett <[email protected]>
This seems to be successfully using x86-latest on Windows now and failing (on purpose) on Ubuntu/Mac. I did this because latest x86-Ubuntu predates 4.6, which is the oldest we are supporting currently. |
Thanks very much for fleshing this out @jaimergp! That one-character change was getting close to the limits of what I might be able to do with javascript. Really appreciate it! |
Any status on this? |
Let's ping core :) @conda-incubator/setup-miniconda |
3 months later this has been approved, but still hasn't been merged. What gives? |
Whoops, this fell under the cracks! Sorry Merging˜! |
A simple fix to what appeared to be a simple bug.
Raise an error if on non-x64 architectures,
miniconda-version
is not specified, instead of when it is specified.