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

Update miniforge version used in installer for OSX #232

Merged
merged 7 commits into from
Dec 2, 2021

Conversation

hmaarrfk
Copy link
Contributor

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@hmaarrfk hmaarrfk requested a review from a team as a code owner November 19, 2021 00:10
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Mark! 😄 Had a couple questions below

build_miniforge_osx.sh Outdated Show resolved Hide resolved
build_miniforge_osx.sh Outdated Show resolved Hide resolved
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

To provide more context to the earlier comments, was thinking something like this. So in the future we really only need to change a couple environment variables

build_miniforge_osx.sh Outdated Show resolved Hide resolved
build_miniforge_osx.sh Outdated Show resolved Hide resolved
@hmaarrfk
Copy link
Contributor Author

Honestly, I do kinda see these repos as places where I would want to point other users to on how to "use" mamba/miniforge.

I don't think I want to install things to a "non-standard" directory yet given that we don't need to. The extra level of indirection is really annoying for new readers.

I think it is a bit of a shame that we can't find a common name for mambaforge and miniforge.

@hmaarrfk
Copy link
Contributor Author

I would prefer avoiding a custom installation location, and avoiding an extra variable if we can.

@jakirkham
Copy link
Member

How is it non-standard? Users are free to set the install path wherever is convenient. In some cases (particularly on systems where the home directory uses NFS), the default is not even a good idea.

@hmaarrfk
Copy link
Contributor Author

I guess non-standard is the wrong word. It just seems "different" than what most people need.

The variable isn't really used beyond this 5 line script. So there is really no point in making it one in my mind.

Again, the main motivation in creating a variable for the version number is it bring it to the "front of the line of code" as opposed to the end of it highlighting its importance.

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Dec 2, 2021

I think you are right. I just want to keep in sync with the anvil images. So I'm trying to follow
https://github.com/conda-forge/docker-images/blob/main/scripts/run_commands

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Dec 2, 2021

seems to pass osx. i hope i got the uname correct,

build_miniforge_osx.sh Outdated Show resolved Hide resolved
# hmaarrfk: 2021/12/01 Not too sure how to check sha256sums on OSX
# shellcheck disable=SC2034
conda_chksum="7c44259a0982cd3ef212649678af5f0dd4e0bb7306e8fffc93601dd1d739ec0b"
elif [ "$(uname -m)" = "arm64" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

@erykoff is this correct for macOS ARM? Or should we have something else here?

Copy link

Choose a reason for hiding this comment

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

Running uname -m on my machine does indeed say arm64.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking! 😄

@jakirkham
Copy link
Member

Seems reasonable. Thanks for the updates Mark 😄

Suggested a change for sha256 (this should work on other platforms as well as long as they have openssl)

Not sure about macOS ARM, but asked someone who may know more above

@hmaarrfk hmaarrfk merged commit b1a7935 into conda-forge:master Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants