-
Notifications
You must be signed in to change notification settings - Fork 31
Fix issues preventing proper packaging of OpenSSL module #76
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
base: main
Are you sure you want to change the base?
Conversation
| "license" : "MIT", | ||
| "description" : "OpenSSL bindings", | ||
| "depends" : [], | ||
| "depends" : { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has some issues.
The this-or-that method of declaring a dependency using array notation is not implemented "depends":[ "Foo", ["Bar:auth<foo>", "Bar:auth<baz>"]]. Last time I mentioned going this route you seemed to have some reservations about it, although I don't remember why -- only that I was in favor of it (just nobody got around to implementing).
The hash form of declaration doesn't accept the string form of names. So using "linux" : "ssl:from<native>:ver<1.1>" probably isn't going to work.
The format of depends is off. Unimportantly I would recommend using the array-of-hash format, but if not then this is still missing a level (we specced runtime etc). More importantly is there is no less-redundant way to properly express what you want (yet). Theoretically it could be like
"depends" : [
{
"from" : "native",
"name" : {
"by-kernel.name": {
"win32": "ssleay32",
"" : "ssl"
}
}
},
{
"from" : "native",
"name" : {
"by-kernel.name": {
"win32": "libeay32",
"":"ssl"
}
}
}
]
but note how we currently have to declare a dependency -- i.e. ssl is listed in the second dependency block even though it is redundant against the first block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oddly zef didn't seem to care about the dependencies when I tried. But maybe I just screwed up my tests. Was a bit distracted by being at FOSDEM and all.
I can't remember what my reservations were about the array notation. Actually I first thought we overlooked this use case and was pleasantly surprised when I found it it S22: "To indicate alternatives, it is possible to specify a list of use strings, instead of just a single use string". If I were asked to review the spec right now, I'd probably not like the implicitness. Usually when you see a list, it's ANDed. You wouldn't assume that your shopping list means "buy beer OR burgers". But then, why would you put a nested list into your list of dependencies? We could spell it {"alternatives": [...]} instead to make it more self documenting. If we could think of another combinator besides AND and OR, that'd make more sense. Or are there other reasonable search strategies besides "the first of these you can find"? If there are, that'd would be an argument for the more elaborate version.
The depends block I specified actually is in array-of-hash format. On e.g. Linux it will collapse to
"depends": [
["ssl:from<native>:ver<1.1>", "ssl:from<native>:ver<1.0>"],
["crypto:from<native>:ver<1.1>", "crypto:from<native>:ver<1.0>"]
]
At least if zef applies system-collapse to the depends value directly.
If you're OK with it I can implement the missing bits in zef. At this point I'm already so many layers deep into yak shaving that one more won't matter ;) We have already done all the hard work. It's about time we reap the benefits and get this stuff used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The depends block I specified actually is in array-of-hash format. On e.g. Linux it will collapse to
I'm not sure this (assumption?) is correct. Collapse does not occur on the top level itself AFAIK. See Inline::Python example and Zef's implementation. If what you did actually collapses as you say then that would seem like a coincidence (if you look at the zef implementation linked is it not some well-designed alternative, but rather some quick make-it-work transformation logic and hence why I suggest using the array-of-hash).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine the issue with alternative syntax was how does one then do both AND and OR
"depends" : ["Foo1", ["Foo2", { { Foo::MaybeThis => 1 }, { Foo::MaybeThat => 1 }}]
We know that we support OpenSSL 1.1 and 1.0, so rather than hoping that /usr/lib64/libssl.so points to a supported version, we should try those first and fall back to the symlink later. This matches the dependency stated in META6.json more closely. Fixes GH raku-community-modules#16 and raku-community-modules#54 and raku-community-modules#59
|
This shouldn't be merged because of the META6.json |
|
I'll try to do it, thanks @ugexe |
|
Sorry for the request, but any progress on this? I am getting annoyed to have to install SSL devel package everytime x) |
|
Looks like this PR has gone stale. However, if someone would like to revive, it could be merged and released pretty quickly now that it's a Raku Community module |
The META6.json file misses information about the license and about the dependency on the native libssl.so library. This PR adds this information. In addition it changes the code for finding the library to first check the versions which we know to work and fall back to the unversioned libssl.so symlink. With this we can get by without an openssl-dev or -devel package installed in many cases.
All of these issues caused pain when packaging the module for Linux distros. With them resolved, the package can be generated fully automatically.