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

Don't use --release flag on Java 8 #213

Merged
merged 1 commit into from Mar 30, 2023
Merged

Conversation

ahorek
Copy link
Contributor

@ahorek ahorek commented Mar 29, 2023

this allows using

  Rake::JavaExtensionTask.new("name", gemspec) do |ext|
    ext.release = '8'
  end

on Java 8 (for building the gem), because the flag is available since Java 9, see https://maven.apache.org/plugins/maven-compiler-plugin/examples/set-compiler-source-and-target.html

this flag is for backward compatibility, so it's safe to just skip it if we can't use it.

relates to puma/puma#3109 socketry/nio4r#292

@@ -303,5 +303,9 @@ def java_lint_arg

"-Xlint:#{@lint_option}"
end

def release_flag_supported?
!(RUBY_PLATFORM =~ /java/) || Gem::Version.new(Java::java.lang.System.getProperty('java.version')) >= Gem::Version.new("9")
Copy link
Member

Choose a reason for hiding this comment

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

Could you use early return for readability?

Suggested change
!(RUBY_PLATFORM =~ /java/) || Gem::Version.new(Java::java.lang.System.getProperty('java.version')) >= Gem::Version.new("9")
return false unless /java/match?(RUBY_PLATFORM)
Gem::Version.new(Java::java.lang.System.getProperty('java.version')) >= Gem::Version.new("9")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I can't use match? because this gem is still ruby 1.8 compatible :) but sure, I'll change it...

Copy link
Member

Choose a reason for hiding this comment

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

We can drop support for Ruby 1.8 because we can't test with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, but it's for a separate PR, what should be the minimum version?

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with a separate PR.

@kou kou changed the title skip release flag on java 8 Don't use --release flag on Java 8 Mar 29, 2023
@ahorek ahorek mentioned this pull request Mar 29, 2023
@kou kou merged commit 8e79814 into rake-compiler:master Mar 30, 2023
12 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.

None yet

2 participants