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

feature: Use archive type header instead of hooks #1210

Conversation

alexanderankin
Copy link

@alexanderankin alexanderankin commented Jun 3, 2023

this begins the work of transitioning away from post_hooks for tar -> zip conversion. If that is all they are used for and the code in this PR works, then i believe removing them is safe. otherwise, other use cases would need to be addressed (or even some old post hooks removed?--- gets complicated).

I also am not 100% sure of how to approach the tests in this repo just yet.


i should probably note that this PR is related to #644 - I've done the parts where:

  1. I add the new header to the wiremock server in the tests
  2. I add the code for verifying and extracting the two archive types - and it seems to pass tests?

I have not done:

  1. manually test the code in this PR - like installing sdkman from source from this branch.
  2. moving files as done in relocation hooks
  3. understanding checksum requirements

todo:

  1. turns out sed \s is a gnu feature - should remove for mac compatibility - use tr -dc '[:alnum:]' instead?

@alexanderankin
Copy link
Author

i wonder if it is safe to assume that all non-tar archives are zips/all tars are correctly labeled - and even if that is not true - the alternatives should be attempted in order rather than guess a default and fail if its wrong.

but just in case, going to leave this code snippet here:
diff --git a/src/main/bash/sdkman-install.sh b/src/main/bash/sdkman-install.sh
index c60ffda..0f90adc 100644
--- a/src/main/bash/sdkman-install.sh
+++ b/src/main/bash/sdkman-install.sh
@@ -189,7 +189,10 @@ function __sdkman_archive_type() {
 
 	# search for the header with a case insensitive match
 	# then use same sed technique as __sdkman_checksum_archive
-	grep -i "$HEADER" "$headers_file" | sed -n "s/^$HEADER:\s*\(.*\)$/\1/p"
+	local -r detected_type=$(grep -i "$HEADER" "$headers_file" | sed -n "s/^$HEADER:\s*\(.*\)$/\1/p")
+
+	# report zips by default? probably not safe...
+	echo -n "${detected_type:-zip}"
 }
 
 function __sdkman_validate_archive() {

@marc0der
Copy link
Member

marc0der commented Jun 3, 2023

@alexanderankin just to let you know that I'm currently rewriting SDKMAN in Rust. The main reason why I haven't spent much energy on fixing the bash scripts is that all of this code is about to die. Once the rewrite is done, you won't need curl, unzip, zip or any other dependency on your machine.

@alexanderankin
Copy link
Author

sure, in the meantime are no PR's allowed since its so close? I would understand but at the same time I figure polished contributions might be worth accepting anyway (this one is still being polished)

@marc0der
Copy link
Member

marc0der commented Jun 3, 2023

That's just FYI, I'm still happy to accept any PRs on the legacy codebase if it makes things better for our users. Also, install, being the most complex command is being left for last. I'm working my way up to the big one 😄

@alexanderankin alexanderankin changed the title WIP: Use archive type header instead of hooks feature: Use archive type header instead of hooks Jun 3, 2023
@alexanderankin alexanderankin marked this pull request as ready for review June 3, 2023 21:12
@hgeraldino
Copy link
Contributor

This looks great.

I did some work on this area a few months back, but the refactor is tricky and the PRs #1156 #1157 went stale.

Basically, removing the post-hooks will break some SDKs (mainly JDKs on MacOS) as they're used not only convert tarballs into zips, but also to relocate some files during this process. Because the changes were a bit involved I created the checksums branch and raise these PRs against it. The desired outcome of this work was:

  • Removing of pre/post-hooks (pre-hooks are gone for good already btw)
  • Enabling checksums for all assets (because of the tar->zip conversion, checksums fail for all tarballs)
  • Adding new 'relocation' hooks to be executed as the very last step of the installation process. These hooks are being served by the sdkman-hooks already, and should handle the cases where files need to be shifted after the zip/tarball file is exploded.

Let me know if you want to pick up this work, but keep in mind that all this will be thrown away (hopefully soon) once the new native clients are released

@alexanderankin alexanderankin marked this pull request as draft June 4, 2023 11:08
@alexanderankin
Copy link
Author

understood - i took a look at the hooks as they are being served today - https://github.com/sdkman/sdkman-hooks/pull/61 and it seems straightforward enough. I don't quite understand where they are being used in the cli today, but i can still implement the logic and eventually qa test when i get there. Not sure if ill be able to knock it all out in one go but we'll see.

@marc0der
Copy link
Member

marc0der commented Jun 4, 2023

If at all possible, I prefer smaller pull requests rather than one single big bang. It makes for easier reviews and decreases the risk of issues. We have a beta environment that is used for these purposes.

@alexanderankin
Copy link
Author

prefer smaller pull requests

yep - when i said i wasnt sure if id be able to do it in one go, the implication was that i might need to resort to too much complexity to accomplish the feature and then overflow into multiple pr's.

will try to provide an update within this week (or close completely)

@alexanderankin
Copy link
Author

couldn't find the time, will need to revisit in the future

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

3 participants