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

Auto-detect OpenSSL 1.1 #2190

Merged
merged 5 commits into from Feb 9, 2019

Conversation

Projects
None yet
7 participants
@wilzbach
Copy link
Member

commented Jul 18, 2018

This is ugly, because dub is quite limited in this regard.
However, the preBuildsCommands + rdmd has been successfully used for a couple of
repositories at dlang-community already.

Also, I don't think this will work on Windows, but if the detection fails, the current behavior (i.e. using OpenSSL 1.0) is maintained.
For Posix-systems this should help a people a lot though and allows a good first five minutes experience.

edit: enabled it only for Posix systems

@wilzbach wilzbach force-pushed the wilzbach:openssl-autodetect branch from fefab1b to cb6975b Jul 18, 2018

@@ -21,6 +21,15 @@ configuration "openssl-mscoff" {

configuration "openssl" {
sourceFiles "../lib/win-i386/eay.lib" "../lib/win-i386/ssl.lib" platform="windows-x86-dmd"
sourceFiles "openssl_version.d"
preBuildCommands `rdmd --eval='

This comment has been minimized.

Copy link
@jacob-carlborg

jacob-carlborg Jul 18, 2018

Contributor

Will RDMD automatically pick LDC/GDC if DMD is not available?

This comment has been minimized.

Copy link
@wilzbach

wilzbach Jul 18, 2018

Author Member

Yes. In fact it will even prefer ldc if ldc is in the same directory.

This comment has been minimized.

Copy link
@Geod24

Geod24 Jul 19, 2018

Contributor

If this script fails, will it fail the build ?

This comment has been minimized.

Copy link
@wilzbach

wilzbach Jul 19, 2018

Author Member

Yes, but that's why the idea is to softly fail to 0.0.0 if no version could be detected. I also added a try/catch to be sure and enabled it for Posix only.

@wilzbach wilzbach force-pushed the wilzbach:openssl-autodetect branch 6 times, most recently from 9d3cb21 to 8f69246 Jul 18, 2018

.travis.yml Outdated
- dmd-2.079.1
- dmd-2.078.3
- dmd-2.077.1
- dmd-2.076.1
- dmd-2.075.1

This comment has been minimized.

Copy link
@wilzbach

wilzbach Jul 20, 2018

Author Member

rdmd --eval doesn't work with 2.075: https://travis-ci.org/vibe-d/vibe.d/jobs/405854762

@WebFreak001

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2018

the new mixin configs based on command output thingy that's being (been?) merged into dub could be useful here right?

@jacob-carlborg

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2018

the new mixin configs based on command output thingy that's being (been?) merged into dub could be useful here right?

Do you have any link for that?

@WebFreak001

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2018

@wilzbach wilzbach force-pushed the wilzbach:openssl-autodetect branch 3 times, most recently from 58a6862 to 996cdfc Jan 31, 2019

@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

the new mixin configs based on command output thingy that's being (been?) merged into dub could be useful here right?

Yep, but it's uncertain whether this will be approved (and it will take quite a while until this dub version is available/used everywhere). In the meantime, this trick works and removes the TLS configuration pain for Posix.

@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2019

@s-ludwig: what's your opinion on this? Or can we finally move ahead with this?
It's been quite a while...

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Feb 2, 2019

We should definitely move forward, I wanted to do that for months now actually...

I mainly have two points:

  • Splitting up the "openssl" configuration is a potentially breaking change, what was the reason for that instead of sticking platform="..." on each declaration as required?
  • I'd probably go ahead one step further and make 1.1 the default if detection fails (adding an "openssl-1.0" configuration to force 1.0)
@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2019

Splitting up the "openssl" configuration is a potentially breaking change, what was the reason for that instead of sticking platform="..." on each declaration as required?

This:

Warning: Multiple configurations with the name "openssl" are defined in package "vibe-d:tls". This will most likely cause configuration resolution issues.

I'd probably go ahead one step further and make 1.1 the default if detection fails (adding an "openssl-1.0" configuration to force 1.0)

Awesome! Done.

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Feb 2, 2019

What I mean is this:

configuration "openssl" {
	dependency "openssl" version="~>1.0"

	// Windows
	sourceFiles "../lib/win-i386/eay.lib" "../lib/win-i386/ssl.lib" platform="windows-x86-dmd"

	// Posix
	sourceFiles "openssl_version.d" platform="posix"
	preBuildCommands `rdmd --eval='
	auto dir = environment.get("DUB_PACKAGE_DIR");
	if (dir.buildPath("tls").exists)  {
		dir = dir.buildPath("tls");
	}
	auto opensslVersion = "0.0.0";
	try {
		const res = execute(["openssl", "version"]).output;
		if (res.canFind("OpenSSL ")) {
			opensslVersion = res.splitter(" ").dropOne.front.filter!(not!(std.uni.isAlpha)).text;
		}
	} catch (Exception e) { writeln("Warning: ", e); }
	text("module openssl_version;\nenum OPENSSL_VERSION=\"", opensslVersion, "\";").
		toFile(dir.buildPath("openssl_version.d"));
	'` platform="posix"
}

I think this should work fine.

Looks like you accidentally added some .orig files in the last commit.

@wilzbach wilzbach force-pushed the wilzbach:openssl-autodetect branch from 2b75b6e to 297e0bd Feb 3, 2019

@wilzbach wilzbach force-pushed the wilzbach:openssl-autodetect branch from 297e0bd to 2c64e3c Feb 3, 2019

@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Feb 3, 2019

I think this should work fine.

Ah I see. Thanks! Updated.

Looks like you accidentally added some .orig files in the last commit.

Yeah I was in a bit of a hurry. Sorry. Fixed.

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Upgraded and fixed the Windows side to work with OpenSSL 1.1, too. Let's hope it passes now.

@s-ludwig s-ludwig force-pushed the wilzbach:openssl-autodetect branch from fa84078 to c31bd06 Feb 8, 2019

@s-ludwig s-ludwig added the auto-merge label Feb 8, 2019

@dlang-bot dlang-bot merged commit ea28640 into vibe-d:master Feb 9, 2019

3 checks passed

codecov/patch Coverage not affected when comparing 5aa20a8...c31bd06
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wilzbach wilzbach deleted the wilzbach:openssl-autodetect branch Feb 9, 2019

@wilzbach

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2019

Awesome. Thanks a lot! I guess we can release 0.8.5 fairly soon then?

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

Definitiely! I'd just try wait for #2247 before tagging the beta (but speaking of which, I'll just tag a new alpha right now).

@bubnenkoff

This comment has been minimized.

Copy link

commented Feb 10, 2019

Can't build latest vibed on Windows 10 with follow error
#2261

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.