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

Improve get_lib_version, use as check #415

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

horstle
Copy link
Collaborator

@horstle horstle commented Dec 23, 2020

Based on the code from @CastagnaIT, this improves the functionality of get_lib_version and adds a check to it. It now returns an empty string if the file is corrupt or the architecture doesn't match.

Currently unit tests fail. I guess that's because they are running on x86 hardware, so the aforementioned check fails. So far I only tested Linux x86 manually.

Closes #413.

@dagwieers dagwieers added arm Related to ARM architecture enhancement labels Dec 28, 2020
@mediaminister
Copy link
Collaborator

Currently unit tests fail.

Strange, maybe related to this: https://github.community/t/segmentation-fault-when-running-binary-in-a-job/18418

@mediaminister
Copy link
Collaborator

@horstle
I found and fixed the problem:

  • the segmentation fault was caused by loading the shared library several times in quick succession. Unloading the library fixes this.
    There was also a problem with loading the 32-bit ARM lib. It throws an OSError with wrong ELF class: ELFCLASS32

I added the old get_lib_version again as a fallback.

Feel free to make a better fix, but this is good enough for me to get merged.

@mediaminister mediaminister added this to the v0.5.3 milestone Jan 6, 2021
@dagwieers
Copy link
Collaborator

Ha, while looking at previous discussions I found my original code from a year ago:

#248 (comment)

@horstle
Copy link
Collaborator Author

horstle commented Jan 7, 2021

@mediaminister It is supposed to fail when there is an arch mismatch, so it won't install the wrong lib. That's what this PR is trying to fix (see #413). Also, the legacy version doesn't detect an arch mismatch, so it can't be used as a check.
Instead of writing worse code to pass the tests, we should improve the tests.

I'm currently looking into AWS CodeBuild, so we can run the ARM tests on ARM hardware. I think it makes sense to have separate Github actions for each platform, then the ARM tests could be run via CodeBuild and maybe at some point the Windows tests could run on Windows (it seems that's already possible in Github, but idk if it would cost something, might look into it later). That way closing the lib wouldn't be necessary, too, although I wouldn't mind keeping that change.

@sonarcloud
Copy link

sonarcloud bot commented Jan 7, 2021

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
9.5% 9.5% Duplication

@mediaminister
Copy link
Collaborator

Okay, I removed the fallback. Tests fail, but now you can see why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arm Related to ARM architecture enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request to better manage case of failed widevine update for Linux ARM
3 participants