ruby-build icon indicating copy to clipboard operation
ruby-build copied to clipboard

Mirror checksums

Open jasonkarns opened this issue 9 years ago • 8 comments

As hinted from #878, this adds mult-digest support to the mirroring script.

In its present form, it adds MD5 and SHA-512 support to the existing SHA-256 behavior. The md5 support is isolated to its own commit, so it can be dropped from this PR if desired. (It was added for completeness sake.)

I'm not really sure how to test this, so it hasn't really been tested :/ Other than the fact it's mostly copied from https://github.com/rbenv/ruby-build/blob/8dc4568ea9f02db55ddf26de9b48d01e0bdb1d05/bin/ruby-build#L267-L278

jasonkarns avatar May 16 '16 15:05 jasonkarns

Kept the local var; I'm not a fan of using positional args without local names because it's not abundantly clear what the variable is. I went with digest_type instead since type is a keyword. And renamed the other use of $digest as well. I wasn't particularly happy with $digest before, anyway, since most people would assume digest to be the actual checksum itself.

Any comment on allowing the mirror to handle MD5?

jasonkarns avatar May 18 '16 13:05 jasonkarns

I'm fine with handling MD5 since it doesn't cost us much.

mislav avatar May 19 '16 11:05 mislav

Just realized I don't think I commented since I made the requested variable name change. I think this is ready to merge unless there is a way to test the mirror script.

jasonkarns avatar Jun 10 '16 14:06 jasonkarns

If MD5 is not needed, best not to include it at all. It's a known-broken algorithm that has numerous problems (and known collisions). Best to stick to SHA-2 and other hash algorithms that are not yet broken.

reedloden avatar Aug 22 '16 01:08 reedloden

@reedloden I'd agree for things that are producing md5. However, this script ought to be able to handle as wide a range of checksums as possible, IMO. That way it can consume the widest possible set of build definition files. Especially because this script operates on git history by taking in ranges of commits as input. Therefore, even though it exists in the repository (and is therefore kept in lockstep with the definition files it consumes), it operates on ranges of history so it rather exists outside of repo history as well. I think it makes sense to be able to operate on old commits that do contain md5 checksums.

jasonkarns avatar Aug 22 '16 01:08 jasonkarns

@jasonkarns Can we find a way to share the checksum functions between these executables? Now that the checksum approach is more sophisticated, I'm not looking forward to maintaining different implementations in different executables.

I'm going to be away for 2 weeks from now, so I won't be around to review/accept PRs. To recognize your substantial contributions to the rbenv family of projects, I've invited you to the org, so you have write access. However, please be wary of merging larger changes (including your own) to either projects without approval of other people in the org. Adding the Rubinius scraper to ruby-build is an example of a larger change that can probably hold off until I come back.

mislav avatar Aug 22 '16 11:08 mislav

@mislav bin/ruby-build already has the lib function which is used to share functions between bin/ruby-build and bin/rbenv-install. We could move the checksum function(s) into lib() so that the mirror script can use them.

However, this brings up another thought I've had for nodenv.

I've long thought that there should be some organizational distinction (probably just different files) between "private" functions that ruby-build uses internally and "public" functions that are intended to be used by build definitions. I was going to suggest making lib/internal.sh and lib/build.sh files to contain those functions (respectively) that would then be sourced by bin/ruby-build.

The current lib() function is crazy clever (nearly a quine!), but I'm not sure if it has any value over just having a separate file that's sourced. Thoughts? Simple solution is just to move the checksum functions into lib and then decide about potential extraction later.

jasonkarns avatar Aug 22 '16 14:08 jasonkarns

Actually, I recognize that keeping ruby-build functional as a single file makes the install step trivial. So I'm still for moving the checksum functions into lib() soon and having a bigger conversation around lib extraction much later.

jasonkarns avatar Aug 22 '16 15:08 jasonkarns

@jasonkarns I'm sorry I never got around to merging this, but I waited to see if ruby-build will need sha-512 support for anything specific, and since it hasn't, I will close this until actually needed.

mislav avatar Aug 08 '23 08:08 mislav