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

Don't crash when target platform is not supported #93

Merged
merged 1 commit into from
Jan 25, 2017

Conversation

arjan
Copy link
Contributor

@arjan arjan commented Jan 25, 2017

This addresses both #89 and #90.

On compile time, a warning is printed when the CPU platform is not supported.

On runtime, while loading the NIF, an error is printed. All NIF
stub functions now return stub values and do not call exit() so that
the integration keeps working (without doing anything) when running on
a non-supported OS.

@jeffkreeftmeijer can you test this somehow?

On compile time, a warning is printed when the CPU platform is not
supporte.

On runtime, while loading the NIF, an error is printed. All NIF
stub functions now return stub values and do not call exit() so that
the integration keeps working (without doing anything) when running on
a non-supported OS.
@arjan
Copy link
Contributor Author

arjan commented Jan 25, 2017

You can partially test this by returning :unsupported in mix_helpers.exs for all archs.
However, the JSON encoding tests will fail. I'm wondering whether we want to expose this data_to_json function in the Appsignal API at all..? People might misuse it as a generic JSON encoding mechanism.

@jeffkreeftmeijer
Copy link
Member

Tested this, as you suggested, by returning :unsupported from Mix.Appsignal.Helpers.map_arch/1.

The data_to_json() function is specifically added to the agent to help with testing the C extension and Nif, and won’t be used elsewhere. You’re right, we shouldn’t export it in the Nif, at least.

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.

None yet

2 participants