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

[Merged by Bors] - Downgrade ADX check to a warning #1846

Closed
wants to merge 1 commit into from

Conversation

michaelsproul
Copy link
Member

Issue Addressed

Closes #1842

Proposed Changes

Due to the lies told to us by VPS providers about what CPU features they support, we are forced to check for the availability of CPU features like ADX by just running code and seeing if it crashes. The prominent warning should hopefully help users who have truly incompatible CPUs work out what is going on, while not burdening users of cheap VPSs.

@michaelsproul michaelsproul added ready-for-review The code is ready for review low-hanging-fruit Easy to resolve, get it before someone else does! A0 labels Nov 2, 2020
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Looks good, modulo one comment that you can choose to address if you like.


## Troubleshooting

If you get a SIGILL (exit code 132), then your CPU is incompatible with the optimized build
Copy link
Member

Choose a reason for hiding this comment

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

I guess this concept also applies to "Build from Source" as well. I notice it doesn't have any info about portability.

I'll leave it up to you to decide if you want to address that here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't apply to source builds unless the user forces ADX on by bypassing the makefile, or moves a binary from an ADX machine to a non-ADX one. For now I'm in favour of merging ASAP, and we can add a note later if people run into it

@michaelsproul
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Nov 2, 2020
## Issue Addressed

Closes #1842

## Proposed Changes

Due to the lies told to us by VPS providers about what CPU features they support, we are forced to check for the availability of CPU features like ADX by just _running code and seeing if it crashes_. The prominent warning should hopefully help users who have truly incompatible CPUs work out what is going on, while not burdening users of cheap VPSs.
@bors bors bot changed the title Downgrade ADX check to a warning [Merged by Bors] - Downgrade ADX check to a warning Nov 2, 2020
@bors bors bot closed this Nov 2, 2020
@michaelsproul michaelsproul deleted the adx-warning branch November 2, 2020 23:45
bors bot pushed a commit that referenced this pull request Nov 2, 2020
## Issue Addressed

NA

## Proposed Changes

- Update versions
- Run `cargo update`

## Additional Info

- Blocked on #1846
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low-hanging-fruit Easy to resolve, get it before someone else does! ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0.3.2: CPU incompatible error with Ubuntu AWS instance
2 participants