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

Updating JAR output path to avoid using long path. #114

Merged
merged 4 commits into from
Jan 25, 2024

Conversation

mashhurs
Copy link
Collaborator

@mashhurs mashhurs commented Dec 19, 2023

Description

Embedding the plugin into core and faced an issue that jar path exceeds path limit or TarWriter we used when assembling.
With this change, we will shorten the JAR path.

BEFORE

268 chars: logstash-8.13.0-SNAPSHOT/vendor/bundle/jruby/3.1.0/gems/logstash-filter-elastic_integration-0.1.2-java/vendor/jar-dependencies/co/elastic/logstash/plugins/filter/elasticintegration/logstash-filter-elastic_integration/0.1.2/logstash-filter-elastic_integration-0.1.2.jar

AFTER

225 chars: logstash-8.13.0-SNAPSHOT/vendor/bundle/jruby/3.1.0/gems/logstash-filter-elastic_integration-0.1.2-java/vendor/jar-dependencies/co/elastic/logstash-filter-elastic_integration/0.1.2/logstash-filter-elastic_integration-0.1.2.jar

@mashhurs mashhurs self-assigned this Dec 19, 2023
@yaauie
Copy link
Member

yaauie commented Jan 8, 2024

The actual changes here LGTM, but if we were to simply add the jar to the vendor directory without any superfluous nesting we may be able to simply use JRuby's Kernel#require_relative, which would shorten the jar's path to:

155 chars: logstash-8.13.0-SNAPSHOT/vendor/bundle/jruby/3.1.0/gems/logstash-filter-elastic_integration-0.1.2-java/vendor/logstash-filter-elastic_integration-0.1.2.jar

@mashhurs
Copy link
Collaborator Author

The actual changes here LGTM, but if we were to simply add the jar to the vendor directory without any superfluous nesting we may be able to simply use JRuby's Kernel#require_relative, which would shorten the jar's path to:

155 chars: logstash-8.13.0-SNAPSHOT/vendor/bundle/jruby/3.1.0/gems/logstash-filter-elastic_integration-0.1.2-java/vendor/logstash-filter-elastic_integration-0.1.2.jar

  • if we were to simply add the jar to the vendor: if we directly place under vendor, the require_jar method (in jar_dependencies.rb) requires three params (group id, artifact id and version). So, we need at least one path for group id. Maybe we can use elastic only? Like elastic - project - version as we organize packaging? How do you think?
  • in a reality, there is not issue in most OS file systems if path is over 256 limit. Pushing TarWriter would make more sense. Unfortunately, there is an open issue lasting over years for this.
  • we may be able to simply use JRuby's Kernel#require_relative: ah this one I couldn't get. Do you mean instead TarWriter we use Kernel#require_relative first and zip?

String vendorPathPrefix = "vendor/jar-dependencies"
String projectGroupPath = project.group.replaceAll('\\.', '/')
File projectJarFile = file("${vendorPathPrefix}/${projectGroupPath}/${project.name}/${project.version}/${project.name}-${project.version}.jar")
String vendorPathPrefix = "vendor/jar-dependencies/co/elastic"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

review note: made a minimal change to shorten the path. We may directly place into vendor but we are using jar-dependencies (in gemspec file) as a required path and include jars as a standard in other Java based plugins.

@mashhurs mashhurs marked this pull request as ready for review January 10, 2024 20:23
@jsvd
Copy link
Member

jsvd commented Jan 25, 2024

the minimal change LGTM too.
I believe @yaauie's suggestion is to replace use of require_jar from jar_dependencies with a plain require or require_relative with the path to the jar, something like (pseudocode)

        jarRequiresFile.withWriter { w ->
            w << "# AUTOGENERATED BY THE GRADLE SCRIPT. DO NOT EDIT.\n\n"
            w << '########################################################################\n' +
                 '# Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V.\n' +
                 '# under one or more contributor license agreements. Licensed under the\n' +
                 '# Elastic License 2.0; you may not use this file except in compliance\n' +
                 '# with the Elastic License 2.0.\n' +
                 '########################################################################\n'
            w << '\n'
            w << "require_relative(::File.join("..", "vendor", "#{project.name}-#{project.version}.jar")\n"
        }

@mashhurs mashhurs merged commit 774169f into elastic:main Jan 25, 2024
1 check 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.

None yet

3 participants