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

Bumped install-nix-action to v23 #1087

Merged

Conversation

jake-arkinstall
Copy link
Contributor

This is because the original version (v20) was not compatible with System Identity Protection on Macos

@cqc-alec
Copy link
Collaborator

Segmentation fault: 11  python -m pytest -s .

:-(

Copy link
Collaborator

@cqc-alec cqc-alec left a comment

Choose a reason for hiding this comment

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

Seems to have solved the first issue but hit a segfault in pytest.

Comment on lines 3 to 9
pull_request:
branches:
- main
- develop
push:
branches:
- develop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once we have it working, I'd prefer not to have this as it could take quite a while to run. Let's just keep it on the weekly schedule (+ possible manual trigger) that we have now.

@jake-arkinstall
Copy link
Contributor Author

I can confirm that nix flake check github:CQCL/tket succeeds on MacOS 13.6. I don't have access to a mac with macos 12

I've now added the -L flag to nix flake check to capture some location information for the segfault.

The issue is happening in the same place as it was on macos13 before the dylib flat namespace patch. I cannot determine whether the issue is the macos version or the CPU architecture.

So this leaves three main options.

One is to resolve the underlying issue, which is that typecast.hpp contains implementation, and that is included into two separate compilation units and is causing the symbol collision. I'm not sure how much pybind11 constrains this, as I believe that implementation does need to be provided in a header.

One is to identify why the vanilla cmake build is working - what's different? Should it be working?

Another is to identify the platforms this currently does work on, and then restrict the support matrix accordingly. This doesn't sound ideal, but might serve as a point to improve on over time.

@cqc-alec
Copy link
Collaborator

So this leaves three main options.

One is to resolve the underlying issue, which is that typecast.hpp contains implementation, and that is included into two separate compilation units and is causing the symbol collision. I'm not sure how much pybind11 constrains this, as I believe that implementation does need to be provided in a header.

One is to identify why the vanilla cmake build is working - what's different? Should it be working?

Another is to identify the platforms this currently does work on, and then restrict the support matrix accordingly. This doesn't sound ideal, but might serve as a point to improve on over time.

All those sound like good options, but for now, if it works on macos-13, I'm happy to just change the platform used in the workflow to macos-13.

@jake-arkinstall
Copy link
Contributor Author

This is now ready for final review

…ot compatible with System Identity Protection
…a diverted store is not supported on this platform' error
…ving cmake debug messages that were used to determine that cmake was detecting the OS correctly
… and corrected cached directory to /nix/store rather than /nix
…ctions/cache runs into permissions problems on the nix store
…sions abilities and the target matrix that we'll need. Github's own actions/cache runs into permissions issues and fails to rebuild the nix store. Determinate Systems' magic nix cache relies on a download that doesn't exist for ARM macos. Long term we should investigate using a public cache (e.g. cachix or host our own) that not only speeds up CI, but also speeds up the process of installing tket for users
@cqc-alec cqc-alec merged commit bacc4e7 into CQCL:develop Oct 30, 2023
30 checks passed
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