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

feat: search for openssl in OPENSSL_LIB_DIR if provided #18012

Closed

Conversation

hlolli
Copy link

@hlolli hlolli commented Feb 21, 2023

This is my first PR here and I approved the prisma CLA.

The reason for this change is that I want to use the native openssl from my unsupported linux distro.
As a compilation environment variable, OPENSSL_LIB_DIR is used when provided in prisma-engines.
It would be very nice if this could be supported on runtime as well. I don't know if there's already a way
to pass in custom openssl lib-dir location (or direct location of the shared library). If there's a way, then
obviously I'll take this PR no further :)

@hlolli hlolli requested review from a team and aqrln and removed request for a team February 21, 2023 00:49
@hlolli
Copy link
Author

hlolli commented Feb 21, 2023

It seems this line here, tough unrelated, prevents this from succeeding

 (path) => `ls -v "libssl.so.0*" ${path} | grep libssl.so | ${excludeLibssl0x}`,

I tried both on osx and linux and

ls -v "libssl.so.0*" ${path}

isn't something that's a valid ls command?

also piping grep into grep without xargs also doesn't work, so I wonder if this part of the function is helpful for what it's intended to do?

@jkomyno
Copy link
Contributor

jkomyno commented Feb 22, 2023

Hi @hlolli, thanks for your PR! Before proceeding further, may you please tell us which Linux distro you're trying to support, which libssl version you have installed, and which CPU architecture it uses?
Moreover, could you please print a screenshot of what would be the content of your folder matching this new env var OPENSSL_LIB_DIR?

Thanks :)

@hlolli
Copy link
Author

hlolli commented Feb 22, 2023

hi @jkomyno

I want to add that prisma still works, but I believe it uses the debian node pre-compiled binary, actually, I'm surprised it works.

The contents of openssl when I use v3 (which I currently do since prisma support it) is

engines-3/  libcrypto.so@  libcrypto.so.3*  libssl.so@  libssl.so.3*  ossl-modules/

The location of this folder is different depending if I'm playing steam (then it's /lib64), or building locally, then it's /home/hlolli/result/lib/ or globally /nix/store/openssl/lib

Personally I use osx, we deploy using nixos on our CI machine

Architecture:            x86_64
  CPU op-mode(s):        32-bit, 64-bit
  Address sizes:         36 bits physical, 48 bits virtual
  Byte Order:            Little Endian
CPU(s):                  8
  On-line CPU(s) list:   0-7
Vendor ID:               GenuineIntel
  Model name:            Intel(R) Core(TM) i7-3770 CPU @ 3.40GHz
  
  $ uname -a
Linux kudos 5.15.93 #1-NixOS SMP Thu Feb 9 10:26:48 UTC 2023 x86_64 GNU/Linux

we deploy to raw docker (non os base-image) on AWS-ECS cluster, I don't have an easy way to ssh into those, but the results from our ci system is 1:1 reporducible.

I wonder if this whole check could be skipped if native binaries are used, because they themselves dynamically link against openssl?

2023-02-22 16 04 38

Something like https://www.npmjs.com/package/elfinfo could be used to track down the location if it's necessary. And would be more reliable imo than hardcoded distro-specific library paths.

@aqrln
Copy link
Member

aqrln commented Feb 24, 2023

Something like https://www.npmjs.com/package/elfinfo could be used to track down the location if it's necessary. And would be more reliable imo than hardcoded distro-specific library paths.

Outside of nix world, normally the full paths to the libraries are not specified in runpath, so it wouldn't be possible to rely on that on other distros. Besides, we'd need to have the binary the read the runpath in the first place, while these checks exist to know which binary to download.

However, these checks are basically meaningless when custom binaries are used, and the printed warnings could even be misleading. I opened an issue about NixOS previously: #17900. I wonder if we could even skip platform detection completely and not just skip printing warnings as that issue suggests.

@aqrln
Copy link
Member

aqrln commented Mar 2, 2023

Also, to clarify about this:

I want to add that prisma still works, but I believe it uses the debian node pre-compiled binary, actually, I'm surprised it works.

Given your screenshot, it looks like you are using the package from nixpkgs, so Prisma doesn't download any binaries, it uses the ones from pkgs.prisma-engines (which are basically custom binaries from the point of view of Prisma).

Regarding the ls command though, I agree that there's something wrong with it:

$ ls -v "libssl.so.0*"
ls: cannot access 'libssl.so.0*': No such file or directory

$ man ls | grep -- -v
     -v      Force unedited printing of non-graphic characters; this is the default when output is not to a terminal.

It doesn't look like it actually breaks anything (the output just contains an error before the listing of the files in the specified directory — it would break though if path was an empty string and we wanted the listing in the current directory), but still not nice and should be fixed. Would you be willing to move those changes to a separate PR so they could be merged regardless of the decision about the env var?

@Gerschtli
Copy link
Contributor

What is the status of this PR? I would love to get rid of this warning and provide the correct path to openssl via nix directly.

@Gerschtli
Copy link
Contributor

Because this PR is quite old and some things changed in prisma, I created another PR #20138 :)

Gerschtli added a commit to Gerschtli/prisma that referenced this pull request Jul 18, 2023
Gerschtli added a commit to Gerschtli/prisma that referenced this pull request Jul 18, 2023
Gerschtli added a commit to Gerschtli/prisma that referenced this pull request Jul 25, 2023
Gerschtli added a commit to Gerschtli/prisma that referenced this pull request Jul 25, 2023
Gerschtli added a commit to Gerschtli/prisma that referenced this pull request Jul 27, 2023
Gerschtli added a commit to Gerschtli/prisma that referenced this pull request Jul 28, 2023
aqrln pushed a commit that referenced this pull request Jul 28, 2023
* feat(get-platform): search for openssl in LD_LIBRARY_PATH if provided

Supersedes: #18012

* fix(get-platform): prevent false positive matches for libssl.so

In `parseLibSSLVersion`, it is expected that the filename contains the
version number, checked with the following regex:

    /libssl\.so\.(\d)(\.\d)?/

With this change, generic `libssl.so` files are not matched anymore.
@aqrln
Copy link
Member

aqrln commented Jul 28, 2023

Thank you for your PR @hlolli and sorry we couldn't bring it to the finish line. We have now merged #20381 which is a more generic alternative to this idea, see #20138 (comment) for the motivation and for the reasoning about why we decided against a specific env var just for OpenSSL.

Also see #20138 (review) for the overview of the current state of NixOS user experience (kudos to @Gerschtli for the patches!) and possible future work.

I'll go ahead and close this PR, please let me know if there's still something relevant that wasn't fixed in other way yet.

@aqrln aqrln closed this Jul 28, 2023
aqrln pushed a commit that referenced this pull request Aug 2, 2023
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.

None yet

4 participants