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

Skip channel program tests on Apple Silicon #17053

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pcd1193182
Copy link
Contributor

Motivation and Context

I have a small VM set up on my macbook to do some basic testing with. Unfortunately, the channel program tests consistently cause me problems. The issue appears to be that there is something wrong with our lua interpreter when running on apple silicon.

Description

This PR simply skips the channel program tests if it detects that it's running on Apple Silicon.

How Has This Been Tested?

Ran the test suite on my macbook and on a normal linux VM.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Copy link
Member

@robn robn left a comment

Choose a reason for hiding this comment

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

Maybe I would make a is_linux_apple_silicon or similar to not have to repeat it. On the other hand, this is a temporary situation, so maybe making a proper function gives a suggestion of permanence that we don't want to encourage :)

Copy link
Contributor

@tonyhutter tonyhutter left a comment

Choose a reason for hiding this comment

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

Thanks, you can squash these and should be good to go.

@adamdmoss
Copy link
Contributor

I really rather believe, FWIW, that if there's "something wrong with our lua interpreter when running on apple silicon" then we should forbid channel program support on apple silicon, not forbid the tests, which seems counterproductive.

@tonyhutter
Copy link
Contributor

@adamdmoss that's a good point. @pcd1193182 what are your thoughts on disabling all channel programs on Apple silicon until we get a real fix?

@robn
Copy link
Member

robn commented Feb 18, 2025

You can't currently disable channel programs; snapshot destruction requires it (see dsl_destroy_snapshots_nvl()).

Regardless, this is a small, targeted test skip for a specific platform that keeps the rest of the test suite running on that platform and so keeps that platform working and useful for development. We do this all the time - we have Linux and FreeBSD skips throughout the test suite.

@tonyhutter
Copy link
Contributor

@pcd1193182 can you give some more detail on the problem you're seeing? Is it a test-case problem, or a real problem with lua on Apple silicon?

@pcd1193182
Copy link
Contributor Author

It's a real problem; if you try to run any lua program that triggers an exception (like the one in the divide by zero test), the setjmp/longjmp mechanism doesn't appear to work, and you end up with a panic on linux.

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.

4 participants