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

Update build to gradle #41

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

Conversation

robbavey
Copy link
Contributor

Update to use gradle to avoid jar-dependencies based weirdness with old version of jruby, and for consistency with other plugins using java dependencies.

Update to use gradle to avoid jar-dependencies based weirdness
with old version of jruby, and for consistency with other
plugins using java dependencies.
Copy link
Contributor

@sihil sihil left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up @robbavey - you're a hero. I've rebased my PR over this and I discovered a couple of things.

.gitignore should probably have /.gradle/ and /build/ included.

I initially had two copies of every jar in my resulting gem but a bundle exec rake clean solved this problem. Possibly worth documenting.

build.gradle Outdated
compile 'com.amazonaws:aws-java-sdk-dynamodb:1.11.14'
compile 'com.amazonaws:aws-java-sdk-s3:1.11.14'
compile 'com.amazonaws:aws-java-sdk-kms:1.11.14'
compile 'com.amazonaws:aws-java-sdk-core:1.11.16'
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is duplicated at line 48.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also there is a mix of 1.11.14 and 1.11.16 - is there a reason for this?

@robbavey
Copy link
Contributor Author

@sihil Thanks!.

I've fixed up the build.gradle to remove the dupes, and added the suggested files to the `.gitignore' file.

The dependencies in the gradle file are essentially taken from the previous build with no attempt, in this PR, to update any of them, bar the commons-logging dep, which was blocking the plugin from building. The intent is to keep this PR as passive as possible, and introduce dependency changes in subsequent PRs.

Also adds further entries to .gitignore
@sihil
Copy link
Contributor

sihil commented May 3, 2018

@robbavey That looks good and makes sense. I've rebased my branch again and it builds happily.

compile 'org.apache.httpcomponents:httpcore:4.4.4'
compile 'com.fasterxml.jackson.core:jackson-annotations:2.6.0'
compile 'com.google.protobuf:protobuf-java:2.6.1'
}
Copy link

Choose a reason for hiding this comment

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

I still know very little about Gradle, so I'm not the best person to review this PR.

But I see the two jar dependencies deleted from the gemspec in here. That's good.

Now my comment: were we not pinning everything else at all? If so, this is a great improvement on build reproducibility. Good stuff!


task :clean do
["vendor/jar-dependencies", "Gemfile.lock"].each do |p|
Copy link

Choose a reason for hiding this comment

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

I may be missing some context, but why delete Gemfile.lock when cleaning? And why is the file in .gitignore?

The lockfile is important to share a very precise known good set of resolved dependencies between developers working on this...

If the issue is that we need to test on multiple incompatible configurations (e.g. JRuby 1.7 & 9.x) and a single Gemfile doesn't cut it, we can always use a the appraisal gem to drive bundler.

Copy link
Member

Choose a reason for hiding this comment

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

we don't have the habit of committing the .lock file in the plugins, so the .lock is listed in the .gitignore.

When a plugin is used with logstash, logstash won't care about any locked dependencies besides the ones in the gemspec. So from this perspective, I think that recomputing the .lock on every test run is more likely to catch dependency-related issues that we would see on logstash, which is a good thing.

@sihil
Copy link
Contributor

sihil commented Jul 3, 2018

Any estimate on when this might get some attention?

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.

5 participants