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

Added Debian bullseye support for the installer #1202

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

marnunez
Copy link

Brief summary of changes

Changed to use ID of OS instead of NAME
Only use VERSION_ID if ID == "ubuntu"

Testing I've completed

Built on my own Debian bullseye/sid

@aymanhab
Copy link
Member

Thanks for submitting this @marnunez, quick feedback:

  1. whitespace/tab changes makes it hard to see the actual diffs, can you fix that?
  2. In our current work-from-home environment, and moving forward it is hard to access/test linux installers, so it would be good to leverage/test the artifacts produced by github actions on actual machines running linux.

@marnunez
Copy link
Author

  1. Sorry about the autoformatting, my bad.
  2. I see what you mean, how would you suggest to approach this? There's no ready github actions build for a Debian environment, but something more fancy using self-hosted runners might be done if needed. As for now, I did build and install this PR with the Debian machine I'm using, for what it's worth...

@aymanhab
Copy link
Member

aymanhab commented Jun 3, 2020

@marnunez If you fix the formatting, and @halleysfifthinc approves the change (to make sure it doesn't break his changes, I don't have access to either platforms) that would be great. Also please include in this PR, adding your name to the list of contributors in the contributing.md file. Thank you

Copy link
Contributor

@halleysfifthinc halleysfifthinc left a comment

Choose a reason for hiding this comment

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

The added whitespace will need to be removed or else it will show up in the printf statements and mess with the output indenting.

Comment on lines +125 to +126
Comment=OpenSim is an open source software for neuromusculoskeletal modeling, simulation \
and analysis.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Comment=OpenSim is an open source software for neuromusculoskeletal modeling, simulation \
and analysis.
Comment=Neuromusculoskeletal modeling, simulation, and analysis.

We had changed this in the standalone desktop file but I noticed that I had forgot to make that change here as well.


if [[ $OS == "Ubuntu" ]] && [[ $VER == "18.04" ]]; then
fi
if ([[ $OS == "ubuntu" ]] && [[ $VER == "18.04" ]]) || ([[ $OS == "debian" ]] && [[ $OS_FULL =~ "bullseye" ]]); then
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason (ie in terms of an expectation of a non-exact match) for using the =~ comparison for matching "bullseye"? I'm not familiar with Debian

Comment on lines +89 to +91
if ! [ -z "${VERSION_ID+x}" ]; then
VER=$VERSION_ID
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

My bash skills aren't the greatest (and I wasn't able to figure it out with some quick googling); can you explain what the +x is doing? Also, if you're negating the -z option, wouldn't using -n or even just [ "$VERSION_ID+x}" ] be more concise/readable?

VER=$VERSION_ID
. /etc/os-release
OS=$ID
OS_NAME=$NAME
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable appears to be unused now and can be removed?

@halleysfifthinc
Copy link
Contributor

Regarding the Actions CI artifacts, I believe what @aymanhab is suggesting is to check that the installer produced by CI works by test installing it on your machine.

@artgodwin
Copy link

I have run the build script under Debian bullseye and found 1 issue :
it needs default-jdk in the preinstalls.

However, the installation crashes very easily including on the examples. Not sure if this is a problem with any older libs included in debian

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.

4 participants