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

Support "os.version" system property in SubstrateVM #557

Closed
fcurts opened this Issue Jul 20, 2018 · 8 comments

Comments

4 participants
@fcurts

fcurts commented Jul 20, 2018

Like the already supported "os.name" system property, "os.version" is often used in informational and diagnostic messages, which makes it worthwhile to support.

@olpaw olpaw self-assigned this Jul 20, 2018

@olpaw

This comment has been minimized.

Member

olpaw commented Jul 20, 2018

@fcurts I will add support for the os.version system property.

@olpaw olpaw added the substrate label Jul 20, 2018

@olegchir

This comment has been minimized.

Contributor

olegchir commented Jul 25, 2018

@olpaw @cstancu is it a high priority target? I could try to implement this.

@olegchir

This comment has been minimized.

Contributor

olegchir commented Jul 25, 2018

Important question on this topic. What's the policy of reusing code from main OpenJDK? I mean, we already have a code for os.version (and many other interesting things), maybe we should just migrate it (with reasonable refactorings)?

@olegchir

This comment has been minimized.

Contributor

olegchir commented Jul 25, 2018

Fixed by #568

The most straightforward solution, because os.version is quite fast.
Should we make it lazier?

@cstancu

This comment has been minimized.

Member

cstancu commented Jul 25, 2018

Reusing OpenJDK code, i.e., copying and pasting it, is ok from a licensing standpoint, however we must have a good reason to do so. If a feature can be implemented by reusing the JDK code without copying it, e.g., patching it through a substitution, then that is preferable.

@cstancu

This comment has been minimized.

Member

cstancu commented Jul 25, 2018

By the way, @olpaw is already working on getting the right os.version at runtime.

@olpaw

This comment has been minimized.

Member

olpaw commented Jul 26, 2018

@cstancu it's in final testing. It will land today (or tomorrow).

@olpaw

This comment has been minimized.

Member

olpaw commented Jul 26, 2018

Fixed in 4529207

This change adds system property support for:

os.version
java.version
java.vendor
java.vendor.url

@olpaw olpaw closed this Jul 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment