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

conf: add exit_on_dl_error option #2828

Merged
merged 1 commit into from
Dec 7, 2021

Conversation

tmatth
Copy link
Contributor

@tmatth tmatth commented Dec 3, 2021

This makes dlopen or dlsym errors optionally fatal, which is especially useful for CI.

@tmatth
Copy link
Contributor Author

tmatth commented Dec 3, 2021

For reference I added this option as I've been burned a few times by plugins, transports, etc. not loading at runtime due to undefined symbols. The latest example was trying to use the websocket transport with libwebsockets built with LWS_WITHOUT_CLIENT=ON, which fails at runtime with:

Couldn't load event handler plugin 'libjanus_wsevh.so': /opt/app/lib/janus/events/libjanus_wsevh.so: undefined symbol: lws_client_connect_via_info

As an aside, does it make sense to set that option in CI here?
https://github.com/meetecho/janus-gateway/blame/1d3f68d094df200d7246a3a349ec50bed1e79985/.github/workflows/janus-ci.yml#L129

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

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

This does sound useful, thanks! Added a couple of notes inline. I'll leave it to @atoppi to comment on whether it might be useful in CI, but in that case my guess is we'd need to add a command line argument to that: if this can only be enabled via config file, not sure we can automate that easily.

janus.c Show resolved Hide resolved
janus.c Show resolved Hide resolved
This makes dlopen or dlsym errors fatal, which is especially useful for CI.
@lminiero
Copy link
Member

lminiero commented Dec 7, 2021

As an aside, does it make sense to set that option in CI here?

@tmatth I just realized you were talking of something entirely different, i.e., the libwebsockets option in the CI build. Again, letting @atoppi address that, though 😆

For the rest, this looks good to me, so I'll merge 👍

@lminiero lminiero merged commit 35704b1 into meetecho:master Dec 7, 2021
@atoppi
Copy link
Member

atoppi commented Dec 7, 2021

@tmatth sorry for being late to the party
Indeed this is an interesting feature and I can see its appliance.
I agree with Lorenzo about the CLI argument, that will be easier for any automation tool in future.

As of

As an aside, does it make sense to set that option in CI here?
https://github.com/meetecho/janus-gateway/blame/1d3f68d094df200d7246a3a349ec50bed1e79985/.github/workflows/janus-ci.yml#L129

The rationale here is to speed-up the building of lws.
We never actually starts janus in the CI workflow (maybe in future we could add this?) since we only check that compilation completes successfully, hence a symbol failure after dlopen can never be triggered.

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

3 participants