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

Reworked setting of bash JAVA_HOME #106

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Johnny-Malizia
Copy link

This change will set JAVA_HOME before a command is executed, not when
the prompt is displayed. The difference is subtle but it reduces
terminal latency significantly.

I also swapped out the call to asdf which for asdf where to reduce the
amount of work necessary to set the JAVA_HOME as the value provided by
asdf where provides the correct path.

@halcyon from our discussion earlier, I think this change fits a little better and keeps all existing functionality only reducing the amount of latency added to the terminal.

This change will set JAVA_HOME before a command is executed, not when
the prompt is displayed. The difference is subtle but it reduces
terminal latency significantly.

I also swapped out the call to asdf which for asdf where to reduce the
amount of work necessary to set the JAVA_HOME as the value provided by
asdf where provides the correct path.
@@ -1,19 +1,20 @@
asdf_update_java_home() {
local java_path
java_path="$(asdf which java)"
java_path="$(asdf where java)"
Copy link
Owner

@halcyon halcyon Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be ok - the reason we switched to which is because it behaves differently when the jdk is set to system.

  % asdf where java
System version is selected
  % asdf which java                                                                                                                                     
/usr/bin/java

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the call to asdf itself is what's slowing things down, so using the where command suffers the same lag. I wonder if there is some sort of callback that we can subscribe to when asdf identifies that it needs to use a new .tools-version.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to follow up on my last train of thought. The callback option may not actually work because the tool versions are "not known" until the shims are actually executed. So when calling something like java -version, asdf doesn't know which installed Java to use until at that very moment. If we were saving the last known Java version somewhere, we could potentially be out of sync because we may have traversed to a different folder already. A callback would not be able to keep the JAVA_HOME up-to-date in the same manner.

So to summarize, this is a pretty tough nut to crack. @smorimoto reviewed my PRs in the past; maybe they could weigh in?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to update this to work when no java is available otherwise JAVA_HOME would get set to whatever asdf where java was outputting.

asdf_update_java_home() {
	local java_path
	if (asdf where java >/dev/null 2>&1); then
		java_path="$(asdf where java)"

		if [[ -n "${java_path}" ]]; then
			export JAVA_HOME
			JAVA_HOME="${java_path}"
		fi
	fi
}

Copy link

@bdellegrazie bdellegrazie Dec 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsmestad / @halcyon :
This may be a stupid question and I apologise if it is but why are you exporting JAVA_HOME before setting it rather than after?

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.

6 participants