-
Notifications
You must be signed in to change notification settings - Fork 47
Build local app bundle #37
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
base: main
Are you sure you want to change the base?
Build local app bundle #37
Conversation
Also auto-detect the architecture of the machine and the JVM. Stop and require the use of x86-64 and Rosetta on aarch64.
|
I may have put too many things together in one PR so let me know if you want me to re-work it. It's including a .class file which is super yucky, but I included the source there, too. The script with re-generate the .class file from the .java file if it's not there, but only if JAVA_HOME points to a JDK... |
| if arch -x86_64 /usr/bin/true 2> /dev/null; then | ||
| echo "Rosetta 2 appears to be installed, so you will already be able to run the x86-64 JRE/JDK." | ||
| else | ||
| echo "You may also need to install Rosetta 2 to run an x86-64 Java Runtime Environment." | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the above echo seems to guide the user that they need to re-run the script, yet these two messages seem to guide them to different steps. is there perhaps a more clear way to surface the information to the user so it's clear what action(s) they need to take?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe.
In order to run IPMIView and use the KVM (which is pretty important), the prerequisites include an x86-64 JVM. You can do that on aarch64 as long as you (a) have the JVM (!!) and (b) have Rosetta 2 installed.
I'm not 100% sure if the "Rosetta 2 detection scheme" I have above will always work. I know it works for me, but it's entirely possible it won't work for others, or maybe it won't work in the future, so I hedged the language with "You may need to install..." rather than stating that it is absolutely a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to be more descriptive. In the first case, Rosetta 2 is definitely installed and all you need is the JVM.
In the second case, you probably need to download and install both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure if the "Rosetta 2 detection scheme" I have above will always work. I know it works for me, but it's entirely possible it won't work for others,
Yeah, that's fair - I'd honestly just lean on the community to help test this detection scheme to confirm one way or the other. I'm not suggesting this NEEDS to happen before this PR gets merged, but if folks are willing/able to ahead of the merge, that'd be great.
How about something like this as a straw man proposal:
if [ "x86_64" != "$jarch" ] ; then
rosetta_install_required=0
if ! arch -x86_64 /usr/bin/true 2> /dev/null; then
echo "[ERROR] On ${arch}, Rosetta 2 is needed to run an x86-64 Java Runtime Environment."
rosetta_install_required=1
fi
echo "[ERROR] The Java architecture for JAVA_HOME=$JAVA_HOME is $jarch. This application bundle requires an x86-64 JRE."
echo
echo "[FAILED] IPMIView.app was not built. Take the below step(s) and re-run the script."
echo " - Set JAVA_HOME to an x86-64 JRE."
if [ $rosetta_install_required -eq 1 ]; then
echo " - Install Rosetta 2."
fi
exit 1There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a look at my most recent changes. I'm happy to change over to a bulleted-list of instructions if you think it's "too much to read".
| echo "Completed." | ||
| echo | ||
| echo "You can now open ~/Applications/IPMIView.app" | ||
| echo "You can now open ./IPMIView.app or copy it into ~/Applications" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we keep this contract (that the script yields ~/Applications/IPMIView.app) the same? any particular reason to change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not married to local-only build, but this is the only product I know of that really wants to put itself into ~/Applications.
There's no reason to require it to go there other than it gets into Launchpad (eventually). You could also install it into /Applications if you chose to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understand - ultimately, it's up to the person running this script to decide where they ultimately want the app to live (in /Applications, or elsewhere). My point is that the change here is moving where it is currently stored, which might be unexpected by users who have known the script to generate the app into ~/Applications directory. If that's something that we really really want to change, then I'd recommend we also update all other references to this, and put a clear notification to folks in the README stating that the location the app is generated to has moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this and maybe make it a completely separate PR.
Use java -XshowSettings:properties instead of custom code
|
@ChrisCarini and @ChristopherSchultz, you have both be added as invited collaborators on this project. Feel free to take it under your collective wings. I'll remain a bystander but will probably not be able to contribute in the near term. If you have any issues with access for making changes, reviewing or merging - please do let me know. |
Thanks @TheCase - you had actually already added me as a collaborator a while back :P But appreciate the sentiment again! ❤️ |
|
Also, if either of you would prefer to host a new active fork under either of your accounts, I can adjust the project notes to direct folks to the project repo that is under active development. |
If that's desired/preferred, I'd be happy to volunteer to take it. I think for the time being, it should be ok - should we run into other problems where we need to consider a transfer/fork, I'd vote we have that conversation then. WDYT @TheCase ? |
|
Works for me :) |
Builds IPMIview.app locally instead of into ~/Applications.
Detects architecture and requires x86-64 on aarm64.
Saves the connection properties from the old application bundle.