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
Merged

Conversation

wilzbach
Copy link
Member

@wilzbach wilzbach 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

@@ -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='
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

If this script fails, will it fail the build ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 openssl-autodetect branch 6 times, most recently from 9d3cb21 to 8f69246 Compare July 19, 2018 15:12
.travis.yml Outdated
- dmd-2.079.1
- dmd-2.078.3
- dmd-2.077.1
- dmd-2.076.1
- dmd-2.075.1
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@WebFreak001
Copy link
Contributor

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

@jacob-carlborg
Copy link
Contributor

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
Copy link
Contributor

@wilzbach wilzbach force-pushed the openssl-autodetect branch 3 times, most recently from 58a6862 to 996cdfc Compare January 31, 2019 15:57
@wilzbach
Copy link
Member Author

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
Copy link
Member Author

wilzbach 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
Copy link
Member

s-ludwig 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
Copy link
Member Author

wilzbach 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
Copy link
Member

s-ludwig 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
Copy link
Member Author

wilzbach 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
Copy link
Member

s-ludwig commented Feb 8, 2019

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

@dlang-bot dlang-bot merged commit ea28640 into vibe-d:master Feb 9, 2019
@wilzbach wilzbach deleted the openssl-autodetect branch February 9, 2019 13:53
@wilzbach
Copy link
Member Author

wilzbach commented Feb 9, 2019

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

@s-ludwig
Copy link
Member

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
Copy link

bubnenkoff 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants