Move the version detection script from vibe-d:tls to deimos/openssl
#66
Conversation
Currently, the importPath is the root directly, meaning that it's impossible to store D source files that are not directly part of the library (such as a D script). Moving to a source directory will allow us to add the vibe.d version detection script here.
|
@CyberShadow : I suppose you will be the most affected by this, given the directory structure change, but without a dedicated source path any script would end up polluting the library. |
|
CC @schveiguy |
|
eh? I haven't had much issues with ssl. I mostly disable it and rely on the wrapper web server (apache) so far. |
|
With this change, is it possible to still use the bindings as a submodule, without Dub? |
|
@CyberShadow : Yes. You would have to:
Point 2 will not work well on Windows, so the bindings will always assume 1.1.0. But that's no worse from what we have currently, can be overridden by the user, or fixed at the binding's level. |
|
@schveiguy : Just saw you in the list of "maintainers" (as in, you did a release). |
|
If I did a release, I probably messed it up lol. @CyberShadow is the real expert here. |
This isn't going to work, because I don't think What other options are there? Can we pregenerate this file for applicable versions, and/or use |
I'm not sure what you mean ? The code depends solely on stdlib so doing Regarding other approaches, we could have |
Sorry, I meant that they don't have any hooks for running an external program before beginning the build. I understood this as that all programs which use these bindings but don't use Dub will need to add or change their build scripts to invoke this generator program, right?
That sounds like the right direction, but I would go even a little further.
What do you think? |
Correct. To be fair, if they don't have such hooks, they aren't much of a build system. But that doesn't mean we should break things for those, obviously.
That makes sense, but at this point, it would be a breaking change. Could we do this in the next major ? Unless you plan to release a major anyway because of the files having moved ?
Question is, should we support every version ? I suggest to only support those listed in the "Table of content" of the changelog. In practice, it means that we'll have
Sounds good to me. |
|
Updated |
Right, "system" is not a good term no describe these. More like method of building sufficiently-simple D software. SSL is such an ubiquitous part of software today though that we should probably aim to keep things as simple as we can.
Yeah, I think this warrants a major version bump.
We technically don't have the resources to actively support even one version, all support is best-effort and user-contributed. I understand that by "support" you mean "keep it in the tree", which to me raises another question - why do we aim to support multiple OpenSSL versions with one bindings version, anyway, instead of using branches? With branches we could essentially "support" any OpenSSL version indefinitely, and users can continue contributing fixes to that OpenSSL version's branch. I can see how Dub's treatment of version tags could make things a little unobvious but it doesn't seem insurmountable. Furthermore, I'm not even sure if it makes sense to define a backwards-compatibility policy based on OpenSSL version numbers. AFAIK, OpenSSL versioning is based on compatibility with C source code, but what we really care about is compatibility with OpenSSL's ABI, which to OpenSSL developers is an implementation detail. On a more practical note, I think we can drop code for older OpenSSL ABIs along with major version bumps of this package, and if someone would like to contribute fixes for an old ABI, we can always revive the last commit before the removal as a new branch and create tags from there. |
After posting I realized that one reason in favor doing so is exactly what this pull request aims to facilitate - targeting varying OpenSSL ABI versions with one Dub configuration file. I don't think Dub has any facilities to calculate a dependency version dynamically based on an external condition, so this is as good as it gets there. Same goes with Git submodules. |
I work on Ubuntu 22.04, my coworkers work on 18.04 or 20.04. The only way we can work together is to use
Yes and no. They have taken a quite flawed approach to compatibility, but they have to maintain forward compatibility with patch versions (what they call build), otherwise shared library linking would not work. I can imagine there might be exceptions (e.g. if we used some private / undocumented API), but overall the public API have to be stable across "build" of the same "version". |
Agreed, this is more of a "best effort" basis. For reference, Vibe.d does this ATM: I simply want to encourage people to send their fixes upstream instead of hacking it downstream. |
There was a problem hiding this comment.
Some questions:
- Is this going to interfere with caching? Such as forcing a rebuild every time?
- What happens if the version of OpenSSL on the build machine is updated? If the file is generated every time, no problem, but otherwise can we make it clearer which file the user needs to delete?
Currently, the importPath is the root directly,
meaning that it's impossible to store D source files that
are not directly part of the library (such as a D script).
Moving to a source directory will allow us to add the vibe.d
version detection script here.
I think in theory this should be achievable by setting sourcePaths (glob these files) to deimos and importPaths (tell the compiler to look here for imports) to ., but I don't know if Dub actually implements this distinction sufficiently meaningfully to allow this.
LGTM otherwise though haven't tried building something with this yet.
| @@ -19,4 +26,6 @@ configuration "unittest" { | |||
| targetType "executable" | |||
| dflags "-main" | |||
| excludedSourceFiles "source/deimos/openssl/applink.d" | |||
| preGenerateCommands `${DUB} scripts/generate_version.d` platform="posix" | |||
There was a problem hiding this comment.
Are we hoping for a Windows developer to come along and fix the script (maybe make it check the DLL version in PATH, or the typical install location)?
I have this function for another project which attempts to find an OpenSSL installation:
string findOpenSsl()
{
auto paths = environment["PATH"]
.split(pathSeparator)
.chain(
cartesianProduct(
only(
``, // current drive root
environment.get("SystemDrive", "c") ~ ":",
environment.get("ProgramFiles", `C:\Program Files`),
environment.get("ProgramFiles(x86)", `C:\Program Files (x86)`),
environment.get("ProgramW6432", `C:\Program Files (x86)`),
),
only(
`\OpenSSL-Win32\bin\`,
`\OpenSSL-Win64\bin\`,
),
)
.map!(t => t[0] ~ t[1])
)
.chain(environment.get("OPENSSL_CONF").dirName.only)
.chain(opensslInstallDir.buildPath("bin").only)
.array;
auto openssl = findExecutable("openssl", paths);
debug writeln("Detected OpenSSL in: ", openssl);
return openssl;
}
string opensslInstallDir()
{
version (Windows)
{
auto key = Registry
.localMachine
.getKey(`SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall`);
foreach (name; key.keyNames)
if (name.startsWith("OpenSSL"))
return key
.getKey(name)
.getValue("InstallLocation")
.value_SZ;
}
return null;
}There was a problem hiding this comment.
Yes that's exactly what I was hoping for. Hoping you'll build on it once it's pushed. But even if no one builds on it, the change is still a net improvement IMO.
source/deimos/openssl/opensslv.d
Outdated
| enum OPENSSL_VERSION_NUMBER = | ||
| OPENSSL_MAKE_VERSION(OPENSSL_VERSION_MAJOR, OPENSSL_VERSION_MINOR, OPENSSL_VERSION_PATCH, OPENSSL_VERSION_BUILD); | ||
| OPENSSL_MAKE_VERSION(OpenSSLVersion.Major, OpenSSLVersion.Minor, | ||
| OpenSSLVersion.Patch, OpenSSLVersion.Build); |
There was a problem hiding this comment.
It seems especially odd that this will be kept up to date, but not the individual constants. If those were to be changed to point to the struct template, then this change becomes unnecessary.
There was a problem hiding this comment.
That's a good point. I decided to update this because OpenSSL has this constant has a way to version code. With our D bindings, I don't think that this is really useful TBH.
There was a problem hiding this comment.
I don't understand what you mean by "OpenSSL has this constant has a way to version code".
There was a problem hiding this comment.
as a way*
I meant that those were designed to work with C code and a preprocessor. Macros only make sense in a binding when they are used to alias an API. A better way to do what OPENSSL_VERSION_NUMBER does in D would be to implement an opCmp so that you could compare two versions.
But anyway, as it's the only point of contention, and I don't use those defs myself, I'll just make them alias the struct fields.
Right, ae has a block like that too. |
The version file is generated every time, but is only written if it is different from what's on disk. So it should not interfere with build systems that take file timestamps into account, and will work if the version is updated (be it upgrade or downgrade). |
|
Marked as resolved the thing that were addressed, commented otherwise, and pushed an updated version. EDIT: And added a comment about the file writing logic. |
This script was originally written for vibe-d:tls module. However, it makes more sense to have the detection done in bindings. Before this commit, the OpenSSL version was just assumed to be 1.1.0h.
|
Updated:
|
There was a problem hiding this comment.
I tested building DFeed against it and it built/linked fine, just needed to update the import path symlink (lib/deimos/openssl from ../deimos-openssl/deimos/openssl to ../deimos-openssl/source/deimos/openssl).
I didn't test the Dub side but I'm guessing you have that covered.
|
Since we aim to provide compatibility with more than one OpenSSL version, I'm thinking the next tag should be just |
|
That makes sense. And the README could probably use an update ? |
The OpenSSL bindings are often annoying to deal with, as different versions will expose slightly different API, which will then fail to link, and there was no good way to detect the version in use. Over the year, Vibe.d has built a script to solve this problem, which relies firstly on
pkg-config, then on theopensslbinary itself.While there could probably be issues with non-conventional setups, using
pkg-configseems like the best way forward. However, even ifvibe-d:tlsdoes the detection, the bindings in here still assume1.1.0. Moving the detection to this repository will hopefully reduce the need for duplicated work in downstream users ofopenssl.