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

Error message for MacOS mixed Racket and Rust install #570

Merged
merged 8 commits into from
May 10, 2023

Conversation

zaneenders
Copy link
Collaborator

On MacOS, detects if an x86 version of Racket is installed on an ARM Mac computers. Then prompts the user to install the Apple Silicon version of Racket.

@zaneenders zaneenders requested a review from pavpanchekha May 8, 2023 23:20
Comment on lines 26 to 27
(define (check-for-rosetta)
(equal? (with-output-to-string (lambda () (system "sysctl -n sysctl.proc_translated"))) "1\n"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move the tests for architecture and OS into here. For two reasons:

  • Make this function safer—that is, it won't execute an unknown command if it is called on Linux or Windows.
  • Logically, if you're not on macOS or you are running in ARM, you're definitely not in Rosetta

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, instead of calling it check-for-rosetta, let's call it running-on-rosetta?. This is a function that returns a boolean.

(equal? (with-output-to-string (lambda () (system "sysctl -n sysctl.proc_translated"))) "1\n"))

(define fallback-message
"Error: unable to load the 'egg-math' library.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This one starts with Error: but the one below doesn't. You should make them match up. Also, why not add a link to the bug tracker and say "Please file a bug here: "?

@zaneenders
Copy link
Collaborator Author

@pavpanchekha ok should be good. Let me know if that's not the logical ordering you were looking for.

(define-ffi-definer define-eggmath (ffi-lib libeggmath-path))
; Checks if Racket is being run in emulation via rosetta
(define (running-on-rosetta?)
(if (and (equal? (system-type 'os) 'macosx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do an if where the else branch is false? It should just be part of the and

(define fallback-message
(string-join
`("Error: unable to load the 'egg-math' library"
"Please file a bug at \"https://github.com/herbie-fp/herbie/issues\"")
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I would drop the quotes around the URL; you don't have them in the other message.

consistent link messages
@zaneenders zaneenders merged commit 1f5aabe into main May 10, 2023
@zaneenders zaneenders deleted the x86-arm-m1-bug branch May 10, 2023 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants