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

Add OpenSSL 1.0.2 prereq on CentOS/RHEL6 for OpenJ9 #760

Merged
merged 1 commit into from
Apr 18, 2019

Conversation

sxa
Copy link
Member

@sxa sxa commented Apr 8, 2019

This can be downloaded live, but I'd rather have it cached. CentOS6 (used on x64 for building) has openssl 1.0.1 which cannot be used by OpenJ9.
Separate PR in to the build repository to start using this adoptium/temurin-build#1023

@sxa sxa added the ansible label Apr 8, 2019
@sxa sxa added this to the 2019 April milestone Apr 8, 2019
@sxa sxa self-assigned this Apr 8, 2019
@sxa sxa added this to TODO in infrastructure via automation Apr 8, 2019
@sxa sxa requested review from jdekonin and karianna April 8, 2019 15:35
- ansible_distribution_major_version == "6"
register: openssl_version
tags: openssl

Copy link
Contributor

Choose a reason for hiding this comment

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

Tags should be modified such that either/or openssl role can be targeted/omitted.

Copy link
Contributor

@jdekonin jdekonin left a comment

Choose a reason for hiding this comment

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

This doesn't appear to be modifying from a system point of view. As in not on the library path or binary path, and this is intended not to be correct?

Assuming the answer to that is yes, then I think an update to the tags and commits squashed and it good. From an OpenJ9 build system perspective v1.1.1 is currently downloaded, but @vsebe has an update via #759 to add v1.1.1 to the system as default to satisfy both current OpenSSL and future JITaas requirements.

@sxa
Copy link
Member Author

sxa commented Apr 9, 2019

@vsebe's change is for JITaaS which I believe needs 1.1.1. This is to allow RHEL/CentOS6 to build against the same level main level (1.0.2) as the other CentOS7.4 systems which have that version preinstalled. I don't want to mess with the system version.

Can you clarify what your issue is with the tags? Your comment is against something that already has an openssl tag on it.

@jdekonin
Copy link
Contributor

jdekonin commented Apr 9, 2019

The current openssl role uses tags: openssl, I just think they should be separated and not grouped into one as they are different roles for different purposes.

@vsebe
Copy link
Contributor

vsebe commented Apr 9, 2019

@sxa555 - #759 installs openssl 1.1.1 on all Linux x86 platforms. Openssl 1.0.2 is not required on centos 6 & rhel 6 x86.

@karianna karianna moved this from TODO to In Progress in infrastructure Apr 10, 2019
@sxa
Copy link
Member Author

sxa commented Apr 10, 2019

@vsebe My preference was to build the openJ9 JDKs against 1.0.2 since that's the earliest release and the one that's consistent with the CentOS7.4 build platforms therefore safer for compatibility. If we're 100% sure that building against 1.1.1 source (I believe that's a JITaaS requirement so not actually needed for any adoptopenjdk builds) will result in something that will execute against 1.0.2 that I'd be ok with not doing this, but I'd need that confirmation.

@sxa
Copy link
Member Author

sxa commented Apr 10, 2019

@jdekonin Is the openssl111 purely for jitaas? In which case a jitaas tag for 1.1.1 might be more appropriate as per my previous comment to Violete

@AdamBrousseau
Copy link
Contributor

afaik 1.1.* is required for JITaaS which is currently only supported on xlinux. I assume Adopt would need this (on at least the compile machines) in order to ship binaries with JITaaS support. Even if we aren't testing JITaaS at Adopt.

@sxa
Copy link
Member Author

sxa commented Apr 10, 2019

@AdamBrousseau I thought the JITaaS was still in a separate branch so we wouldn't even be building it at present, or am I out of date? :-)

@karianna
Copy link
Contributor

@sxa555 Will need rebasing

@AdamBrousseau
Copy link
Contributor

True, I was confusing it with a separate project. JITaaS is still not in the mainline.

@sxa
Copy link
Member Author

sxa commented Apr 10, 2019

@AdamBrousseau In which case we should probably get @vsebe to modify her 1.1.1b upgrade PR (if we decide to land it) to use jitaas instead of openssl although I'm still not convinced we should even have it in the playbooks if it's not required for anything in the mainline code.

@karianna karianna moved this from In Progress to QA / Review in infrastructure Apr 13, 2019
@sxa
Copy link
Member Author

sxa commented Apr 15, 2019

@jdekonin Given the discussions and modifications in 1.1.1 can you update your review please?

@jdekonin
Copy link
Contributor

The merge needs review; there are 3 commits in this PR, 2 of which don't belong. Travis failure is highlighting trailing white space that should also be fixed.

@sxa
Copy link
Member Author

sxa commented Apr 15, 2019

The merge commit shouldn't be an issue as it'll get squashed on merge. Have resolved whitespace

Copy link
Contributor

@karianna karianna left a comment

Choose a reason for hiding this comment

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

Couple of Nitpicks - also should we standardise on OpenSSL 1.0.2r for all platforms in that case?

- ((openssl_version.stdout == '') or ((openssl_version.stdout != '') and (openssl_version.stdout | version_compare('1.0.2', operator='lt', strict=True))))
tags: openssl

- name: Remove downloaded packages for OpenSSL v1.1.0g
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that a correct name?

Copy link
Member Author

Choose a reason for hiding this comment

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

In general I'd rather use the build-system provided OpenSSL at whatever level that is. At the moment RHEL6 doesn't have any suitable one, so I'm only installing this explicitly on RHEL/CentOS6 to make a level of 1.0.2 available to build against (the exacty level isn't actually important and it'll never be the system SSL).

Copy link
Member Author

Choose a reason for hiding this comment

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

No :-) Well spotted!

@@ -0,0 +1,58 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

main.yml~? Seems odd

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a backup file that shouldn't have been in my add - removing ...

@sxa sxa force-pushed the openssl102 branch 2 times, most recently from 431a678 to a2f8731 Compare April 16, 2019 18:14
@sxa sxa requested review from karianna and jdekonin April 16, 2019 19:04
Copy link
Contributor

@jdekonin jdekonin left a comment

Choose a reason for hiding this comment

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

The c96f81de commit shouldn't be there at all, being squashed won't change the fact that it appears in this pr, even when its a non-change.

I don't think you meant to add the OpenSSL role back in did you? That role installs openssl v1.1.1b and was just recently removed in #759

Signed-off-by: Stewart Addison <sxa@uk.ibm.com>
@jdekonin jdekonin merged commit 67a7c1e into adoptium:master Apr 18, 2019
infrastructure automation moved this from QA / Review to Done Apr 18, 2019
@sxa sxa deleted the openssl102 branch December 24, 2020 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants