I chose not to use source to avoid possible name collisions with existing (or future) variables in install_deps.sh (As this is my first time modifying script shells I just followed the advice from here). Plus the current version of the script already parses os-release in similar way (grep + sed) on line 4434.

I assume you're talking about elif. In this case, yes, but then I'll need to add an extra line to put an fi to close the first if (unless there is some other way to do it).

Not to my knowledge. I'll change it.

There is also another missing from the case opensuse-tumbleweed). But it doesn't change the result in the case of opensuse because bash returns 0 implicitly when there is no explicit return.

I removed the other return by mistake, I think, in some last minute changes. (it exists in my first version of the patch D10061).

I'll put it back and add another one at the end of the function.

I'll use case here too.

I agree. But in this case, wouldn't be better to split and change the name of the function is_rpm_based (which now I think is misleading) to is_fedora/is_opensuse/is_redhat, as the current version of install_deps.sh (without my patch and if not for the current bug) really supports only Redhat, Fedora and OpenSUSE (and the blender build documentation lists only Fedora and OpenSUSE`).

I think that a separate test function per distribution will make the code more explicit about what the script really supports (without adding comments).

Fair enough. The sed command is incomplete, though. For example, quotes around the values aren't mandatory. Ubuntu uses ID=ubuntu here. This works:

echo $(sed -n 's/^ID="\?\(.*\)"\?/\1/p' /etc/os-release)

The same is true for the check on VERSION_ID. It's likely to be quoted, but since it's just shell notation, it's not mandatory in all cases. See man os-release for more info.

Yes, that's what I meant. Having an extra fi line is fine by me.

I added the -r flag to remove the need for escaping the special characters ( and ?.

When I tested the provided new regex expression I found that .* also captured the last quote (because it's now optional).

I restricted the capture group to the allowed characters listed in man os-release to avoid this issue.

I replaced is_rpm_based_distro by a simpler and specific opensuse test is_opensuse. This way only opensuse specific codepaths will be impacted by the new changes.

I said before that I'll use case to keep the patch consistent with regard to using case or if. But since I removed is_rpm_based_distro (and with it the other case occurence) I didn't see why I still had to use it.

Tell me if I should change it.

In theory, and currently, this shouldn't happen. I added this case just to make it clear that the code handles only leap and tumbleweed.

But in case it would happen, I think stopping the script is not ideal.

Adding packman repo is an optional step, and the user can skip it if they want to. Stopping the script here will remove that choice.

The url returned by get_opensuse_packman_repo_url() is shown to the user in a message on line 4462 before adding it to zypper's managed repositories.

If the user sees the value 'UNKNOWN' in the message, they can always skip this configuration step and do it manually (with yast or zypper if they know what is the correct url). Also, in my opinion, showing the value UNKNOWN to the user in the context of this script is acceptable, since the target audience are developers.

Still, maybe instead of stopping the script, the message on line 4462 should be changed to warn the user if this case should ever happen (or/and direct them to some instructions on how to add the packman repo manually. A link to openSUSE's wiki for example).

I added it for clarity, it won't work and it won't happen. If the script reaches rpm_flavour that means either redhat-release exists or is_opensuse() evaluates to true.

Tell me if I should remove it?

Done.

There are other places where http is used instead of https (like on line 4441).

Tell me If I should change those too in this patch.