Skip to content

Add temp dir property for native library#4533

Merged
merlimat merged 3 commits into
apache:masterfrom
equanz:add_temp_dir_property_for_native_library
Feb 4, 2025
Merged

Add temp dir property for native library#4533
merlimat merged 3 commits into
apache:masterfrom
equanz:add_temp_dir_property_for_native_library

Conversation

@equanz

@equanz equanz commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

Motivation

If the default temporary directory is not available, native libraries cannot be loaded.

For this situation, other projects have the ability to set custom working directory.
I would like to add such feature to BookKeeper.

examples:

Changes

  • Add temp dir property for native library

@dlg99 dlg99 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Idea lgtm but please refactor this into a reusable utility method so the code is not repeated 3 times.

@equanz
equanz force-pushed the add_temp_dir_property_for_native_library branch 3 times, most recently from f1ed167 to a7d31a8 Compare December 10, 2024 10:39
@equanz

equanz commented Dec 10, 2024

Copy link
Copy Markdown
Contributor Author

@dlg99
I created the new submodule native-library-common and try to add it to shade target.

I'm not sure why native-io is not shaded, but I only added native-library-common.
In my understanding, native-library-common (and guava) is imported even if shaded beacuse native-io requires it.
To avoid this issue, for example, shade native-io as well, or create a class for native-io like 7131e51 .

What do you think?

@equanz

equanz commented Dec 11, 2024

Copy link
Copy Markdown
Contributor Author

I decided to remove guava from native-library-common.

@dlg99 PTAL

@equanz
equanz requested a review from dlg99 December 11, 2024 12:36
@dlg99

dlg99 commented Dec 11, 2024

Copy link
Copy Markdown
Contributor

LGTM but please check what's up with the CI / why PR validation is failing

@equanz equanz closed this Dec 12, 2024
@equanz equanz reopened this Dec 12, 2024
@equanz
equanz force-pushed the add_temp_dir_property_for_native_library branch from 2a402a6 to d43b749 Compare December 12, 2024 09:32
@equanz

equanz commented Dec 12, 2024

Copy link
Copy Markdown
Contributor Author

A failed job runs on Ubuntu 24.04.1 LTS.

Conversely, succeeded jobs run on Ubuntu 22.04.5 LTS.

It's probably due to the actions/runner-images#10636.

I tested it in below.

I am now considering downgrading the image version to ubuntu-22.04. However, we need to address this issue.

@equanz

equanz commented Dec 12, 2024

Copy link
Copy Markdown
Contributor Author

Addressed #4533 (comment) issue in #4536.

@equanz
equanz force-pushed the add_temp_dir_property_for_native_library branch from d43b749 to aa45676 Compare December 20, 2024 07:02
@equanz
equanz force-pushed the add_temp_dir_property_for_native_library branch from aa45676 to af44b07 Compare December 30, 2024 07:26
@equanz

equanz commented Dec 30, 2024

Copy link
Copy Markdown
Contributor Author

rerun failure checks

@equanz

equanz commented Dec 30, 2024

Copy link
Copy Markdown
Contributor Author

@dlg99
Sorry for late. I've addressed it. PTAL

Note:
BookKeeper CI / OWASP Dependency Check failed, but I think this issue is not introduced in this PR.

Run mvn -q -B -ntp clean install verify -Powasp-dependency-check -DskipTests -pl '!stream/distributedlog/io/dlfs,!tests'
Error:  Failed to execute goal org.owasp:dependency-check-maven:10.0.2:aggregate (default) on project bookkeeper: 
Error:  
Error:  One or more dependencies were identified with vulnerabilities that have a CVSS score greater than or equal to '7.0': 
Error:  
Error:  vertx-core-4.5.7.jar: CVE-2024-8391(7.5)
Error:  vertx-grpc-4.5.1.jar: CVE-2024-8391(7.5)
Error:  

@equanz

equanz commented Jan 8, 2025

Copy link
Copy Markdown
Contributor Author

@dlg99
PTAL
#4533 (comment)

@equanz
equanz force-pushed the add_temp_dir_property_for_native_library branch from af44b07 to 5016bf4 Compare January 19, 2025 06:48
@equanz

equanz commented Jan 21, 2025

Copy link
Copy Markdown
Contributor Author

@dlg99 dlg99 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good!

@equanz
equanz force-pushed the add_temp_dir_property_for_native_library branch from 5016bf4 to 4565568 Compare February 2, 2025 02:40
@equanz

equanz commented Feb 2, 2025

Copy link
Copy Markdown
Contributor Author

rebased to current master (0748423)

@merlimat
merlimat merged commit 1e2c6ef into apache:master Feb 4, 2025
@equanz
equanz deleted the add_temp_dir_property_for_native_library branch February 5, 2025 00:23
@hangc0276 hangc0276 added this to the 4.18.0 milestone May 29, 2026
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.

4 participants