openssl: add bash dependency #1251
Conversation
|
@nazar-pc should this be rewritten in python? |
|
It "only runs on Linux", so I see very little point in rewriting the generator.sh script in python for portability. But if you're going to check for dependencies I'd check for perl and GNU Make too... |
neheb
force-pushed
the
rev
branch
2 times, most recently
from
c878803 to
f0aca06
Compare
| # Copy meson.build template file | ||
| subprocess.run(["cp", "../../../meson.build.tmpl", "config/"]) | ||
|
|
||
| # Clone the upstream OpenSSL repository | ||
| subprocess.run(["rm", "-rf", "openssl"]) | ||
| subprocess.run(["git", "clone", "--depth", "1", "--branch", "openssl-" + openssl_version, "https://github.com/openssl/openssl.git"]) | ||
|
|
||
| # Remove the config/archs directory | ||
| subprocess.run(["rm", "-rf", "config/archs"]) | ||
|
|
||
| # Run the make command with LANG set to C | ||
| os.environ['LANG'] = 'C' | ||
| subprocess.run(["make", "-C", "config"]) | ||
|
|
||
| # Copy generated files back into the correct place | ||
| find_cmd = 'mkdir -p ../../../generated-$(dirname "$1"); cp "$1" ../../../generated-"$1"' | ||
| subprocess.run(["find", "config/archs", "-name", "meson.build", "-exec", "sh", "-c", find_cmd, "_ignored", "{}", ";"]) | ||
| subprocess.run(["find", "config/archs", "-name", "*.asm", "-exec", "sh", "-c", find_cmd, "_ignored", "{}", ";"]) | ||
| subprocess.run(["find", "config/archs", "-name", "*.c", "-exec", "sh", "-c", find_cmd, "_ignored", "{}", ";"]) | ||
| subprocess.run(["find", "config/archs", "-name", "*.h", "-exec", "sh", "-c", find_cmd, "_ignored", "{}", ";"]) | ||
| subprocess.run(["find", "config/archs", "-iname", "*.s", "-exec", "sh", "-c", find_cmd, "_ignored", "{}", ";"]) |
There was a problem hiding this comment.
I still do not understand the purpose of this PR.
But this right here.
This is not rewriting the generator from bash into python. This is just... no.
There was a problem hiding this comment.
A proper rewrite would be much more complex to deal with globbing and whatnot.
There was a problem hiding this comment.
Ok?
So what exactly is wrong with the bash version.
There was a problem hiding this comment.
Alpine CI is lightweight and does not come with bash by default. It's probably not a good idea to mandate bash.
There was a problem hiding this comment.
Install a normal userland on the alpine CI then? idk what the problem is.
bash is more lightweight than python, that's for sure.
There was a problem hiding this comment.
but python is needed for meson anyway.
There was a problem hiding this comment.
@eli-schwartz updated version and ported more of these calls to python.
There was a problem hiding this comment.
There's still a bunch of find -execs in there...
There was a problem hiding this comment.
I came up with
+def copy_files(extension):
+ for root, _, files in os.walk("config/archs"):
+ for file in files:
+ if file.lower().endswith(extension.lower()):
+ source_path = os.path.join(root, file)
+ relative_path = os.path.relpath(source_path, search_dir)
+ destination_dir = os.path.join("../../../generated", os.path.dirname(relative_path))
+ destination_path = os.path.join(destination_dir, file)
+
+ os.makedirs(destination_dir, exist_ok=True)
+ shutil.copy(source_path, destination_path)
+
# Change to the directory of the Bash script
script_dir = os.path.dirname(os.path.abspath(__file__))
os.chdir(script_dir)
@@ -47,12 +59,11 @@ os.environ['LANG'] = 'C'
subprocess.run(["make", "-C", "config"])
# Copy generated files back into the correct place
-find_cmd = 'mkdir -p ../../../generated-$(dirname "$1"); cp "$1" ../../../generated-"$1"'
-subprocess.run(["find", "config/archs", "-name", "meson.build", "-exec", "sh", "-c", find_cmd, "_ignored", "{}", ";"])
-subprocess.run(["find", "config/archs", "-name", "*.asm", "-exec", "sh", "-c", find_cmd, "_ignored", "{}", ";"])
-subprocess.run(["find", "config/archs", "-name", "*.c", "-exec", "sh", "-c", find_cmd, "_ignored", "{}", ";"])
-subprocess.run(["find", "config/archs", "-name", "*.h", "-exec", "sh", "-c", find_cmd, "_ignored", "{}", ";"])
-subprocess.run(["find", "config/archs", "-iname", "*.s", "-exec", "sh", "-c", find_cmd, "_ignored", "{}", ";"])
+copy_files("meson.build")
+copy_files(".asm")
+copy_files(".c")
+copy_files(".h")
+copy_files(".s")
I don't think it's equivalent.
neheb
force-pushed
the
rev
branch
6 times, most recently
from
23a27d4 to
9e18b27
Compare
|
I'd feel a lot more comfortable if @nazar-pc okay'ed this, as the person most likely to be updating it in the future. |
|
If we want to make it usable on Windows, this makes some sense, otherwise not really. This is exactly that kind of stuff Bash is well suited for: running a series of simple CLI commands one after another. I don't really know Python, so maintaining Bash would probably be easier and it is arguably nicer to read than a bunch of While I appreciate the effort, I'm not convinced this is better or necessary. |
|
Goal is Alpine compatibility. The Python code currently is not much different than the bash script. |
|
|
Yeah but the original PR simply installed bash on Alpine (and the PR title still says that's what it does). So it's not clear why the goal suddenly became to rewrite it in python (especially if that causes it to be less easy for people working on updating the openssl wrap to do so). "Install the bash package" is pretty compatible with Alpine, no? |
|
Sure but it’s an extra dependency. |
What about removing the script's bashisms so that it runs on Alpine's sh? Doesn't look that hard. |
Do tell. |
|
IIRC, the python script ran faster when I tested it. May have been a fluke. |
|
Or just install bash? |
|
Still waiting to hear what the POSIX sh version of the BASH_SOURCE bashism is. |
Not exactly as useful as |
| for d in glob.glob("../../../generated-config/archs/*/asm_avx2"): | ||
| shutil.rmtree(d) | ||
| for d in glob.glob("../../../generated-config/archs/*/*/crypto/buildinf.h"): | ||
| os.remove(d) |
There was a problem hiding this comment.
maybe go over a list of globs and remove everything within in a nested loop
neheb
force-pushed
the
rev
branch
4 times, most recently
from
388fa72 to
43048a0
Compare
neheb
force-pushed
the
rev
branch
3 times, most recently
from
9dd166f to
149a98e
Compare
generator.sh relies on bash. Gives a better error message when not found. Signed-off-by: Rosen Penev <[email protected]>
generator.sh relies on bash. Gives a better error message when not found.