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

JAVA_HOME on Xonsh shell #113

Closed
Kaylebor opened this issue Nov 10, 2020 · 4 comments
Closed

JAVA_HOME on Xonsh shell #113

Kaylebor opened this issue Nov 10, 2020 · 4 comments

Comments

@Kaylebor
Copy link
Contributor

Hello everyone,

I've been using Xonsh shell for a while, in combination with asdf, and I've found myself with an issue related with JAVA_HOME.

Since I've used this plugin in the past, I checked for the set-java-home.bash script, which didn't work.
So, I've created a set-java-home.xsh script, which fixed the issue for me, using Xonsh events (on_chdir and on_post_init).

In short: how would I go about adding that script to the repo? It's the first time I've tried to contribute to an open source project, so I'm slightly lost on that.
Git is not an issue, I've already got a separate branch ready.

@delgurth
Copy link
Contributor

delgurth commented Apr 5, 2021

@Kaylebor since you are way more knowledgable around xonsh then I'm. Do you have an idea how I can make the xonsh tests in #142 fail? Adding an exit _.rtn after https://github.com/halcyon/asdf-java/pull/142/files#diff-1db27d93186e46d3b441ece35801b244db8ee144ff1405ca27a163bfe878957fR76 and https://github.com/halcyon/asdf-java/pull/142/files#diff-1db27d93186e46d3b441ece35801b244db8ee144ff1405ca27a163bfe878957fR127 didn't do anything, so I removed that again. And with the other shells, if the grep command fails, the test fails.

@Kaylebor
Copy link
Contributor Author

Kaylebor commented Apr 6, 2021

Ey @delgurth

Looking around a bit, I believe there are two ways:

  • raise SystemExit(echo $JAVA_HOME | grep -q adoptopenjdk-8.0.252+9.1.openj9-0.20.0 err>out)
  • sys.exit(echo $JAVA_HOME | grep -q adoptopenjdk-8.0.252+9.1.openj9-0.20.0 err>out)

raise is the recommended choice (internally it calls to sys.exit); grep -q means to not print anything; err>out is equivalent to 2>&1 in bash (which is also available in xonsh).

Note that sys.exit requires sys to be imported, while raise doesn't.

I've tested it a bit with a local script, and it returned the proper error code expected from grep to the shell, both from bash and xonsh.

I've suggested the appropriate edit; tell me if it works (or doesn't)

@delgurth
Copy link
Contributor

delgurth commented Apr 6, 2021

@Kaylebor Thanks a lot, also for the explanation of your suggestion. With the help of the raise SystemExit I found the proper setting which does what I need it to do:

$RAISE_SUBPROC_ERROR = True

I coudn't get your exact code to run, at least not with the xonsh version which was installed in the https://github.com/nektos/act container. But the $RAISE_SUBPROC_ERROR = True is IMHO the better solution, since that way you no longer have to check error status of any command.

I like your addition of the -q option. Why output something you are not interested in.


@halcyon I think this issue can be closed.

Could you please check #136 (simple but nice fix) and my pull requests. Merging all 4 of my pull requests will most likely trigger merge-conflicts because I changed the same file a few times. You won't need to fix those, I'll do that.

@Kaylebor
Copy link
Contributor Author

Kaylebor commented Apr 6, 2021

Glad to be of help, I prefer your solution too (specially since it works 😅).

Plus, with $RAISE_SUBPROC_ERROR it behaves closer to other shells, which makes the code easier to follow. That's always nice.

@halcyon halcyon closed this as completed Apr 6, 2021
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

No branches or pull requests

3 participants