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

make cmake work on macos #2060

Merged
merged 1 commit into from
Jul 11, 2023
Merged

make cmake work on macos #2060

merged 1 commit into from
Jul 11, 2023

Conversation

nathantsoi
Copy link
Contributor

Summary

cmake's downloads (ctags and toolchain) were not working on macos.

Validation

tested on mac os 13.4.1 and cmake 3.26.4

@nathantsoi
Copy link
Contributor Author

nathantsoi commented Jul 10, 2023

One caveat here. I found that the ctags for macOS on the Arduino github releases page is not 64-bit, however, the ctags binary distributed in the Arduino app is... so I also had to:

cp /Applications/Arduino.app/Contents/Java/tools-builder/ctags/5.8-arduino11/ctags ~/.Arduino_Core_STM32_dl/d726370579d207be3e43211ba220e5b1/dist/ctags

I'm not sure if a 64-bit ctags binary is available outside of the Arduino installer, but this would be a better solution.

@fpistm fpistm requested a review from massonal July 10, 2023 05:32
@massonal
Copy link
Contributor

From cmake_host_system_information, redirecting to CMAKE_HOST_SYSTEM_NAME:

Name of the OS CMake is running on.

On systems that have the uname command, this variable is set to the output of uname -s. Linux,
Windows, and Darwin for macOS are the values found on the big three operating systems.

Did you really observe the value "macOS"?! (That's fine, but it would mean the docs are confusing...)

As for the caveat you outline: ctags-5.8-arduino11-x86_64-apple-darwin.zip is what you're looking for, no? Or do you mean they packaged a 32-bits app with a 64-bits name?

@fpistm fpistm added this to In progress in STM32 core based on ST HAL via automation Jul 10, 2023
@fpistm
Copy link
Member

fpistm commented Jul 10, 2023

As an extra information: which Mac-OS version you used ?

@nathantsoi
Copy link
Contributor Author

MacOS version is Ventura 13.4.1

And yes, @massonal, the x86_64 zip contains an i386 executable:

$ wget https://github.com/arduino/ctags/releases/download/5.8-arduino11/ctags-5.8-arduino11-x86_64-apple-darwin.zip
$ unzip ctags-5.8-arduino11-x86_64-apple-darwin
Archive:  ctags-5.8-arduino11-x86_64-apple-darwin.zip
  inflating: ctags
$ file ctags
ctags: Mach-O executable i386

vs. The correct, 64-bit version included with the arduino app:

$ file /Applications/Arduino.app/Contents/Java/tools-builder/ctags/5.8-arduino11/ctags
/Applications/Arduino.app/Contents/Java/tools-builder/ctags/5.8-arduino11/ctags: Mach-O 64-bit executable x86_64

@fpistm
Copy link
Member

fpistm commented Jul 10, 2023

@nathantsoi you don't answer this question from @massonal

Did you really observe the value "macOS"?! (That's fine, but it would mean the docs are confusing...)

On our side a MacBook Pro with Monterey v12.6.1 is used to validate and result of uname -s is Darwin.
It seems strange the result was changed and CMAKE documentation was not updated.
Looking on Ventura, it is always based on Darwin kernel. Ventura version 13.4.1 is based on 22.5.0 Darwin kernel.

@nathantsoi
Copy link
Contributor Author

Correct, I get the string macOS:

(Snippet from FindArduinoCtags.cmake)

cmake_host_system_information(
  RESULT HOSTINFO
  QUERY OS_NAME OS_PLATFORM
)
list(GET HOSTINFO 0 HOST_OS)
list(GET HOSTINFO 1 HOST_ARCH)
message(HOST_OS=${HOST_OS})

Results in:

HOST_OS=macOS

@massonal
Copy link
Contributor

Thanks for the confirmation @nathantsoi.
Then, it seems like it's a bug on CMake side (they probably forgot to update the doc at some point? 🤷 )
I suggest you raise a ticket by them... https://github.com/Kitware/CMake#reporting-bugs

As for the issue you have with ctags, do you see any functional impact from the 32-bits version?
I think here we'll try to stick to official releases as much as possible...
You can nonetheless raise an issue there too to ask for clarification! https://github.com/arduino/ctags

My final take on this PR: it does not introduce any bug, so it may be merged safely.
I'll let @fpistm decide on whether to merge it or wait on upstream fixes in the tools.

Copy link
Contributor

@massonal massonal left a comment

Choose a reason for hiding this comment

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

This code is bug-less and harm-less.
Merging it is a matter of policy.

@nathantsoi
Copy link
Contributor Author

Re. the macOS string, I'll open a ticket w/ cmake.

Re. ctags, I should clarify that the 32-bit version of ctags will not run on macOS 13.x.

Using the https://github.com/arduino/ctags binary results in this error:

/bin/sh: /Users/ntsoi/.Arduino_Core_STM32_dl/d726370579d207be3e43211ba220e5b1/dist/ctags/ctags: Bad CPU type in executable

Hence the need to copy in the 64-bit version from the Arduino.app

Perhaps this can be noted in the cmake setup documentation for now? I am happy to do this if you would like.

@nathantsoi
Copy link
Contributor Author

Actually, @massonal, I just checked

message(CMAKE_HOST_SYSTEM_NAME=${CMAKE_HOST_SYSTEM_NAME})

And this does yield the documented result: CMAKE_HOST_SYSTEM_NAME=Darwin

Note, this is different from the snippets in ensure_core_deps.cmake and FindArduinoCtags.cmake (#2060 (comment))

@massonal
Copy link
Contributor

That last item with CMAKE_HOST_SYSTEM_NAME really is confusing... cmake_host_system_information does redirect to it.

OS_NAME
New in version 3.10.

See CMAKE_HOST_SYSTEM_NAME

Definitely mention that in the ticket you write to CMake!

@fpistm fpistm merged commit 8ff274a into stm32duino:main Jul 11, 2023
20 checks passed
STM32 core based on ST HAL automation moved this from In progress to Done Jul 11, 2023
@fpistm
Copy link
Member

fpistm commented Jul 11, 2023

Perhaps this can be noted in the cmake setup documentation for now? I am happy to do this if you would like.

Feel free to submit a PR for that 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants