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

[wpiutil] Remove RuntimeDetector and greatly simplify RuntimeLoader #6600

Merged
merged 1 commit into from May 24, 2024

Conversation

spacey-sooty
Copy link
Contributor

Resolves #6598

@spacey-sooty spacey-sooty requested review from PeterJohnson and a team as code owners May 8, 2024 03:43
@ThadHouse
Copy link
Member

We can just use System.loadLibrary() where the old calls were. CombinedRuntimeLoader will not work for loading individual libraries.

RuntimeLoader.loadLibrary() might need to stay to give a better error message, but it'd be much simpler then what was already there.

@spacey-sooty spacey-sooty force-pushed the removeruntime branch 8 times, most recently from df7aa18 to e2a3bdf Compare May 8, 2024 12:58
@spacey-sooty
Copy link
Contributor Author

We can just use System.loadLibrary() where the old calls were. CombinedRuntimeLoader will not work for loading individual libraries.

RuntimeLoader.loadLibrary() might need to stay to give a better error message, but it'd be much simpler then what was already there.

Based off debugging my typo in the AprilTagJNI I'd say the error message isn't that bad but they could be worse in different cases idk

@spacey-sooty spacey-sooty force-pushed the removeruntime branch 2 times, most recently from 00449e4 to 503e5c1 Compare May 8, 2024 13:24
@PeterJohnson
Copy link
Member

Isn't the error message for raw loadLibrary() just "can't load native library"? I believe the reason we had the more descriptive error was for the (fairly common) case when users didn't have an updated MSVC runtime library installed.

@spacey-sooty
Copy link
Contributor Author

Yes the error message is just can't load library name here if the updated MSVC runtime is an issue I can add back a simpler version of RuntimeLoader.loadLibrary()

@PeterJohnson
Copy link
Member

Let’s do that, yes.

@spacey-sooty spacey-sooty changed the title [wpiutil] Remove RuntimeLoader and RuntimeDetector [wpiutil] Remove RuntimeDetector and greatly simplify RuntimeLoader May 10, 2024
@spacey-sooty spacey-sooty force-pushed the removeruntime branch 4 times, most recently from 1bb4783 to cc64845 Compare May 12, 2024 05:03
@PeterJohnson
Copy link
Member

Do any vendor libraries use the removed functionality?

@ThadHouse
Copy link
Member

I don't think so. They shouldn't be doing so anyway if they are.

@PeterJohnson PeterJohnson merged commit 6220c6b into wpilibsuite:main May 24, 2024
25 checks passed
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.

Remove RuntimeLoader and RuntimeDetector
3 participants