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

Allow build on macOS #145

Merged
merged 2 commits into from
Sep 15, 2023
Merged

Allow build on macOS #145

merged 2 commits into from
Sep 15, 2023

Conversation

chrisoro
Copy link
Contributor

@chrisoro chrisoro commented Sep 15, 2023

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

The changes introduced in 0.21.0 broke the ability to build this on macOS (running 13.5.2 with newest xcode). This fixes these issues.

  • BlockMemory class had a user-defined copy constructor that tried to copy a std::mutex object, which is not allowed. How did this compile on Windows? Regardless the constructor wasn't used here anyways, so I deleted it.
  • Sadly, Apple still doesn't fully support C++20 (let alone C++17), so std::jthread is not available.
    • Added proper thread management for std::thread in ~XcpLogFileWriter() (only for apple)
    • Changed creation of collector_thread to std::thread (only for apple)
    • stop_thread() could already handle jthread and thread
  • Removed total in XcpLogFileReader.next_block because it was unused and that resulted in a compiler warning
  • For some reason pybin11 doesn't pick up on c++, so I had to force it in setup.py. There are several issues in the pybind github but nothing solved. This is the least intrusive workaround.

I only tested it on macOS but it worked fine.

@christoph2
Copy link
Owner

Interesting,
I didn't expect any Apple users.
Really no C++20 support?
My companion project pya2ldb will soon get
a comprehensive update which heavily relies on C++20 -- coroutines are really a killer feature, unfortunately there's currently no PyBind11 binding.
XcpLogFileReader would than naturally map to a Python generator.

@chrisoro
Copy link
Contributor Author

I've only run this on Windows because the entire tool chain based on Vector is exclusively available for that platform. However, I'm developing a Python project that uses pyxcp, and I work on a Mac. Consequently, when I tried to install pyxcp in my environment, it failed after the release of 0.21.1. This update should resolve the issue.

Do you have any plans to offer wheels for Mac? Specifically, wheels for macosx_13_0_arm64 (which would cover all Macs running on native Apple Silicon) would be fantastic.

@christoph2
Copy link
Owner

Wait a minute!
XcpLogFileReader::next_block IS used as an alternative to C++ coroutines...
Could you please re-enable the code?

@chrisoro
Copy link
Contributor Author

I don't understand what you mean.

I didn't make any changes to next_block, aside from removing the unused auto keyword. My modifications were solely related to the jthread components, which Apple's clang does not support. For reference, you can check: https://en.cppreference.com/w/cpp/20.

@christoph2
Copy link
Owner

OK, just enabled GH action MacOS build from my mobile. I'm not an Apple User, so I won't be able to fix any build problems...

@christoph2
Copy link
Owner

Instant fail, seems PyBind11 related.

@christoph2
Copy link
Owner

Yes, you're right, next_block is still there.

@chrisoro
Copy link
Contributor Author

Instant fail, seems PyBind11 related.

Yup, and this is what my change here fixes :) You started it on master which will naturally fail.

@christoph2 christoph2 merged commit 98190e3 into christoph2:master Sep 15, 2023
14 of 15 checks passed
@chrisoro chrisoro deleted the patch-1 branch September 15, 2023 13:35
@chrisoro
Copy link
Contributor Author

The build job is failing due to the runner employing an outdated version of macOS.
For detailed reference, please click here.
The embedded Xcode version lacks certain C++ features essential for the build.

Could you update the macOS version on the runner? As per GitHub's official documentation, specifying macos-13 instead of the default macos-latest (which equates to version 12) should resolve the problem. While it's in beta, it appears to be a viable solution to explore.

@christoph2
Copy link
Owner

The used MacOS compiler still has a problem with
Line #485, I'll have a closer look if I'm back home.
Here's the build result.

@christoph2
Copy link
Owner

Build environment switched from MacOS 12.6.8 to 13.5.1, the issue persists, but I think it's not a big deal to fix.

@chrisoro
Copy link
Contributor Author

I see that you have fixed the issue and provided a macOS wheel with the newest 0.21.4 release. Thank you for the quick response! :)

@christoph2
Copy link
Owner

One proactive note:
While working with Vector Examples, esp. DAQ related stuff, you will encounter ERR_ACCESS_LOCKED responses.
On Windows this could easily fixed by adding a Vector supplied seed-and-key .DLL to your configuration file, but
on MacOS (and Linux) you're obviously out of luck.
But I'm currently working on a completly new IPython style configuration system, which permits code objects, i.e. a .DLL
is not required if the algorithm is known.
Search your Vector examples directory, e.g. 'C:\Users\Public\Documents\Vector\CANape Examples 21.0\ , for
*.sks files, this is basically the seed-n-key algorithm used.
Yes, it works and will publish the new cfg. system as soon as possible.

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