Skip to content

(PA-1941) Add OpenSSL 1.1.0 #82

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

Merged
merged 15 commits into from
Aug 6, 2018

Conversation

caseywilliams
Copy link
Contributor

@caseywilliams caseywilliams commented Jul 26, 2018

This PR is a WIP; Do not merge This is ready to merge!

@mcdonaldseanp
Copy link
Contributor

jenkins please test with puppet-agent#master on aix61-POWERfa,debian9-64a,debian9-32a,fedora26-64a,fedora27-64a,osx1012-64a,osx1013-64a,redhat5-64a,redhat6-64a,redhat6-32a,redhat7-64a,solaris11-64a,windows10ent-32a

@caseywilliams
Copy link
Contributor Author

jenkins please test this with puppet-agent#master on aix61-POWERfa,arista4-32a,cumulus25-64a,debian8-64a,debian8-32a,debian9-64a,debian9-32a,fedora26-64a,fedora27-64a,fedora28-64a,osx1012-64a,osx1013-64a,redhat5-64a,redhat6-64a,redhat6-32a,redhat7-64a,redhat7-POWERa,redhat7-AARCH64a,redhatfips7-64a,sles11-64a,sles11-32a,sles12-64a,sles12-POWERa,solaris10-64a,solaris10-SPARCa,solaris11-64a,solaris11-SPARCa,ubuntu1404-64a,ubuntu1404-32a,ubuntu1604-64a,ubuntu1604-32a,ubuntu1604-POWERa,ubuntu1804-64a,windows10ent-32a,windows2012r2-64a

@caseywilliams caseywilliams force-pushed the PA-1941/openssl-1.1.0 branch 3 times, most recently from 0ebc02b to 5f4b6af Compare August 2, 2018 23:58
Sean P. McDonald and others added 13 commits August 2, 2018 16:59
This commit migrates build requirements from pkg.build_requires statements in
component files to platform files.

As part of this, statements have been added pruning out EL builds from using
pkg.build_requires at all.
This commit changes the location of build requirements from component
configuration to platform configuration.
This commit moves the build requirements from component files to the platform
files.
This commit updates the netdev and fedora 25 platforms to use platform files
to specify build requirements.
This commit removes all pkg.build_requires statements from components that
reference external packages.

We have migrated all external package deps to platform files, where we can
have more control over them
There was a typo in a regex check.
@caseywilliams caseywilliams force-pushed the PA-1941/openssl-1.1.0 branch from 5f4b6af to eb16100 Compare August 2, 2018 23:59
OpenSSL is failing to operate on aarch64 linux when built in a standard
format. Per suggestion of OpenSSL themselves
https://github.com/openssl/openssl/blob/OpenSSL_1_1_0h/INSTALL#L678L680
this commit patches the ssl configuration to use -O2 instead of -O3 when
compiling the tool. This seems to have fixed the issue
@caseywilliams
Copy link
Contributor Author

puppetlabs/puppet-agent#1503 is required for puppet-agent#master for when this is merged and promoted

@caseywilliams
Copy link
Contributor Author

caseywilliams commented Aug 3, 2018

Here are some green agent acceptance tests of puppet-agent#master using openssl 1.1.0 (with AIX 7.1 and 7.2 failures due to my own configuration mistakes - those have passed separately before, though, and I'll retest them tonight). I also confirmed that the agent 5.5.x, PDK, and bolt runtimes still build for all of their supported platforms with the build_requires migrations that have been made here.

@caseywilliams caseywilliams changed the title (PA 1941) Add OpenSSL 1.1.0 (PA-1941) Add OpenSSL 1.1.0 Aug 4, 2018
@@ -18,9 +28,8 @@
echo 'deb http://osmirror.delivery.puppetlabs.net/debian/ wheezy main
deb http://osmirror.delivery.puppetlabs.net/debian/ wheezy-updates main' >> /etc/apt/sources.list
apt-get update -qq
apt-get install -qy --no-install-recommends build-essential make quilt pkg-config debhelper devscripts rsync
apt-get install -qy --no-install-recommends build-essential make quilt pkg-config debhelper devscripts rsync #{packages.join(' ')}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, but would be nice to include build-essential, make, etc. in the packages array too.

@@ -0,0 +1,145 @@
component 'openssl' do |pkg, settings, platform|
Copy link
Contributor

@ekinanp ekinanp Aug 4, 2018

Choose a reason for hiding this comment

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

There's some common stuff here w/ the other openssl.rb file. Least the ENVIRONMENT, FLAGS, TARGETS + INSTALL STEPS. Could there be a _base_openssl.rb file?

@geoffnichols
Copy link
Contributor

So great to see build_requires consolidated into the platform configs here!!

💯

@mcdonaldseanp mcdonaldseanp force-pushed the PA-1941/openssl-1.1.0 branch from 9a9814b to 3e4231b Compare August 6, 2018 16:11
@mcdonaldseanp
Copy link
Contributor

The few style changes requested are probably valid, but we need to move forward with ruby so IMO we should merge this

@branan
Copy link
Contributor

branan commented Aug 6, 2018

Given we're going to have OpenSSL 1.0 and 1.1 side-by-side for quite a while (the lifetime of 5.5.x), should we factor out a base openssl class? Did the configuration end up different enough between the two that it's not worth it?

@@ -140,11 +116,27 @@
# -Sean P. McDonald 07/01/2016
lib_type = platform.architecture == "x64" ? "seh" : "sjlj"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you replaced usage of lib_type with gcc_postfix - should this line be removed?

@@ -140,11 +116,27 @@
# -Sean P. McDonald 07/01/2016
lib_type = platform.architecture == "x64" ? "seh" : "sjlj"

if platform.architecture == "x64"
gcc_postfix = 'seh'
Copy link
Contributor

Choose a reason for hiding this comment

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

Super bikesheddy nitpick: would suffix be better?

Copy link
Contributor

Choose a reason for hiding this comment

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

ha, yeah.

if platform.architecture =~ /aarch64|ppc64le|s390x/
pkg.build_requires "runtime-#{settings[:runtime_project]}"
pkg.environment "PATH", "/opt/pl-build-tools/bin:$(PATH):#{settings[:bindir]}"
pkg.environment "CFLAGS", settings[:cflags]
pkg.environment "LDFLAGS", settings[:ldflags]
end
elsif platform.is_deb?
pkg.build_requires 'libreadline-dev'
if platform.name =~ /debian-9/
pkg.requires 'libreadline7'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these just get bounced up to the platform level as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe pkg.requires if for runtime requirements to be added to the package, so I think these need to stay.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I think about it, this... actually probably does nothing, since we build the runtime as a tarball and not as a full package?

@@ -90,7 +79,6 @@
pkg.environment "PKG_CONFIG_PATH", "/opt/csw/lib/pkgconfig"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ideally we'd want to move all these environment variables up to platform files too, but that might be too much for this PR :)

Copy link
Contributor

Choose a reason for hiding this comment

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

There's also no way to force environment from platform files, we would need to add that to vanagon

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we need to enable that - a ton of the duplication across components is environment variables

Copy link
Contributor

@branan branan left a comment

Choose a reason for hiding this comment

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

The only one of my comments I really want to see addressed is the dead code lib_type. The others are more "open questions and improvements for the future"

@mcdonaldseanp mcdonaldseanp force-pushed the PA-1941/openssl-1.1.0 branch from 894afe5 to 5ce6df8 Compare August 6, 2018 17:39
@branan branan merged commit 1a62356 into puppetlabs:master Aug 6, 2018
witjoh pushed a commit to witjoh/puppet-runtime that referenced this pull request Feb 24, 2021
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