Skip to content

Update dependency and load native libraries without unpacking from archive #17145

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

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

Conversation

dkocher
Copy link
Contributor

@dkocher dkocher commented Jun 2, 2025

Fix #17143.

@dkocher dkocher added this to the 9.2 milestone Jun 2, 2025
@dkocher dkocher requested a review from AliveDevil June 2, 2025 15:41
@dkocher dkocher requested a review from a team as a code owner June 2, 2025 15:41
@dkocher dkocher requested a review from Copilot June 9, 2025 10:04
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the dependency configuration to load native libraries without needing to unpack them from an archive, while also updating related build properties and dependency references.

  • Adds a new property for the updated jansi version in pom.xml.
  • Moves the configuration for native library loading in C# from the constructor to the setDefaults method and updates native library paths.
  • Updates platform-specific pom.xml files and build.xml to reference the new jansi dependency and associated native library types.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pom.xml Added new property for the jansi dependency version.
core/src/main/csharp/ch/cyberduck/core/preferences/ApplicationPreferences.cs Relocated the native library path configuration to setDefaults.
cli/src/main/csharp/ch/cyberduck/cli/WindowsTerminalPreferences.cs Configured jansi native library path for Windows terminals.
cli/pom.xml Switched to using the jansi-min artifact with the new version.
cli/osx/pom.xml Expanded native library inclusion types and added architecture-specific jansi dependencies.
cli/osx/build.xml Updated JVM arguments to include the new library.jansi.path property.
cli/linux/pom.xml Added architecture-specific jansi dependencies for Linux.
cli/dll/pom.xml Added architecture-specific jansi dependencies for DLL loading.
Comments suppressed due to low confidence (2)

cli/osx/build.xml:26

  • Please add a comment explaining the purpose of the 'library.jansi.path' argument to facilitate future maintenance and understanding of the build configuration.
value="-client --add-opens=java.base/sun.security.ssl=ALL-UNNAMED --add-opens=java.base/sun.security.util=ALL-UNNAMED -Djava.library.path=$APP_PACKAGE/Contents/Frameworks -Dlibrary.jansi.path=$APP_PACKAGE/Contents/Frameworks -Djna.boot.library.path=$APP_PACKAGE/Contents/Frameworks -Djna.library.path=$APP_PACKAGE/Contents/Frameworks -Djna.nounpack=true -Djava.awt.headless=true -Dsun.jnu.encoding=utf-8 -Dfile.encoding=utf-8 -Dsun.io.useCanonCaches=false -DLog4jContextSelector=org.apache.logging.log4j.core.selector.BasicContextSelector -XX:+UseG1GC -XX:MinHeapFreeRatio=10 -XX:MaxHeapFreeRatio=20 -XX:+UseStringDeduplication"

cli/pom.xml:82

  • [nitpick] Verify that switching from 'jansi' to 'jansi-min' is intentional and that 'jansi-min' provides the required functionality without causing compatibility issues across environments.
<artifactId>jansi-min</artifactId>

@@ -145,6 +143,8 @@ protected override void setDefaults()
{
base.setDefaults();

Copy link
Preview

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

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

Consider adding a comment to document why the native library path is now set in setDefaults instead of the constructor, to clarify the change in initialization order.

Suggested change
// The native library path is set here in setDefaults instead of the constructor
// to ensure that it is initialized after the base defaults are set. This change
// in initialization order avoids potential issues with uninitialized properties.

Copilot uses AI. Check for mistakes.

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.

Update jansi dependency
1 participant