-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add support for opensuse #3298
base: master
Are you sure you want to change the base?
Add support for opensuse #3298
Conversation
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.
This is a really great start; thanks! Maybe this will beat gentoo and arch :)
The O of croagh!
# | ||
# You should have received a copy of the GNU General Public License | ||
# A copy of the GNU General Public License can be downloaded from | ||
# http://www.gnu.org/licenses/gpl.html |
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.
We can't include GPL'd code in crouton, unfortunately. Why do you need to emulate debootstrap's semantics, anyway?
You'll need to delete this code from the commit history before the pull request can be merged.
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.
Ok, I'll see how to resolve this.
host-bin/enter-chroot
Outdated
systemd_logind="/usr/lib/systemd/systemd-logind" | ||
fi | ||
|
||
if [ -x "$CHROOT${systemd_logind}" ] && \ |
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.
nit: don't need { }
here and in other places
installer/openSUSE/bootstrap
Outdated
# directory), $INSTALLERDIR/$DISTRO, $RELEASE, $BOOTSTRAP_RELEASE (if different | ||
# from $RELEASE), $ARCH, and $MIRROR. | ||
|
||
CUR=`pwd` |
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.
unused?
installer/openSUSE/defaults
Outdated
# It must set at least ARCH and MIRROR if not already specified. | ||
|
||
if [ -z "$ARCH" ]; then | ||
ARCH="`uname -m`" |
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.
use $( )
instead (here and elsewhere).
Yes, the code you're referencing still has it, but we're moving away from that on all new/modified code.
installer/openSUSE/defaults
Outdated
esac | ||
|
||
if [ -z "$MIRROR" ]; then | ||
MIRROR="${CROUTON_MIRROR_opensuse_arm:-http://download.opensuse.org/ports/}" |
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.
Is this the right mirror for x86?
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.
No, but actually MIRROR is not used anywhere. I'll delete it.
targets/xorg
Outdated
@@ -43,7 +43,7 @@ inteldriver='' | |||
intelhasfbc='' | |||
pinmesa='' | |||
intelfbcsupport='y' | |||
if [ "${ARCH#arm}" = "$ARCH" ]; then | |||
if [ "${ARCH#arm}" = "$ARCH" ] && [ "${ARCH#aarch64}" = "$ARCH" ]; then |
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.
oh, this looks like a bug...could you make a separate PR for this? I wonder if we've done similar mistakes elsewhere.
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.
...or is it just that debian/ubuntu refers to it as arm64?
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.
Well in install/debian/defaults we set ARCH to arm64 while I did set it to aarch64. We should unify this. I tend to use aarch64, as arm64 is only a kernel thing, but I don't care too much honestly.
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.
let's be consistent with the other distros and use arm64
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.
ok, I'll fix installer/opensuse/getrelease.sh
targets/xorg
Outdated
@@ -68,7 +68,7 @@ if [ "${ARCH#arm}" = "$ARCH" ]; then | |||
fi | |||
|
|||
# Catalog relevant and irrelevant video drivers | |||
fbdev="xserver-xorg-video-fbdev$backport" | |||
fbdev="openSUSE=xf86-video-fbdev,xserver-xorg-video-fbdev$backport" |
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 wonder if it would be cleaner to have each distro set the xorg package name prefix at the top and then just reference $xorg-video-fbdev
(etc) without needing any distro modifiers
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 don't see any schema that would be easy to implement. Some have a prefix, some have a postfix and some have a different name.
targets/xorg
Outdated
@@ -97,7 +97,7 @@ fi | |||
|
|||
# On Freon, we need crazy xorg hacks | |||
if [ -n "$freon" ]; then | |||
compile freon '-ldl -ldrm -I/usr/include/libdrm' so libdrm-dev | |||
compile freon '-ldl -ldrm -I/usr/include/libdrm' so openSUSE=libdrm-devel,libdrm-dev |
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.
same goes for devel vs. dev, I guess. You don't have to make any changes, though.
targets/xorg
Outdated
@@ -181,7 +181,7 @@ if [ -n "$backport" ]; then | |||
"xserver-xorg-input-synaptics$backport" $installvideodrivers | |||
fi | |||
|
|||
if release -lt stretch -lt kali-rolling -lt yakkety; then | |||
if release -lt stretch -lt kali-rolling -lt yakkety -lt tumbleweed; then |
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.
-lt tumbleweed
means any opensuse release that is earlier than tumbleweed, not including tumbleweed itself. Is that what you meant?
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.
note that if you don't want this for any opensuse release, you can just leave the line unmodified (without -lt tumbleweed
) and it won't match opensuse.
targets/xorg
Outdated
@@ -194,12 +194,12 @@ if [ -z "$inteldriver" -a -n "$backport" ] && release -eq precise; then | |||
# so replace it with something not broken | |||
install_dummy xorg -- xserver-xorg$backport libgl1-mesa-glx$backport \ | |||
libgl1-mesa-dri$backport libglu1-mesa xfonts-base x11-apps \ | |||
x11-session-utils x11-utils x11-xfs-utils x11-xkb-utils \ | |||
x11-session-utils openSUSE=xprop,x11-utils x11-xfs-utils x11-xkb-utils \ |
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.
is xprop the only thing we need from x11-utils?
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.
yes, AFAIK it is the only tool needed
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
238fd08
to
60145c0
Compare
CLAs look good, thanks! |
Just a friendly ping that I send a new pull request in here (not sure if I screwed up because we lost the comments on the old one). So any comments are welcome :) |
installer/opensuse/debootstrap.sh
Outdated
@@ -0,0 +1,76 @@ | |||
#!/bin/sh -e | |||
# Copyright (c) 2017 The crouton Authors. All rights reserved. |
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.
You can't claim copyright just by deleting code, and most (all?) of this file appears to be copied from here.
There's also no reason to fake the interface of debootstrap. Please make an effort to implement opensuse/bootstrap without the use or reference of this code. I suspect you can write what you need from scratch in a few lines inside opensuse/bootstrap.
7e8d768
to
303d26f
Compare
I updated the pull request. I dropped debootstrap and implemented the necessary bits directly in bootstrap. Also I added two commits for chromium (un)support. Happly awaiting any comments :) |
# from $RELEASE), $ARCH, and $MIRROR. | ||
|
||
# Tumbleweed | ||
url_tumbleweed_armv7hl="http://download.opensuse.org/ports/armv7hl/tumbleweed/images/openSUSE-Tumbleweed-ARM-JeOS.armv7hl-rootfs.armv7l-Current.tbz" |
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.
how did you decide on these URLs? The sizes are dramatically different (the X11 ones are 800 megs!). There're also distribution/openSUSE-current/appliances/ and distribution/openSUSE-stable/appliances/ directories with similar images.
There are unversioned entries in the leap subdirectory. Are those always updated? Are we going to have to keep updating the tarball in this file if we specify a version?
And what happens on i386/x86_64?
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.
JeOS images are minimal images (Just Enough Operating System). For armv7hl no plain JeOS image exists. I'm investigating this. Maybe we can just delete the armv7hl support for Leap42.2 and add it in a later patch.
Leap images don't get updated, they get released and that's it. The unversioned images are links to the versioned ones. I'll update the links, as the version is of no use.
We should try to add a automatic resolution of architecutre + os-version -> download link, but I would prefer to leave this for a latter patch, e.g. when adding Leap-42.3 support. TBH I'm not too confident that this will work out, as the folder structure of the download server might change for newer releases.
i386/x86_64 images don't exists at this point in time, only the installer ISO is created.
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.
so as we can't support i386 right now, I will delete the architecutre from installer/opensuse/defaults as well.
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.
Ok, armv7hl JeOS image got created now, I will fix the URL.
installer/opensuse/bootstrap
Outdated
BOOTSTRAP_RELEASE="$RELEASE" | ||
|
||
# Rename factory to tumbleweed | ||
[ "$RELEASE" = "factory" ] && BOOTSTRAP_RELEASE=tumbleweed |
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.
please convert this to a full if statement
installer/opensuse/bootstrap
Outdated
[ "$ARCH" = "arm64" ] && BOOTSTRAP_ARCH=aarch64 | ||
|
||
key=$(echo "\$url_${BOOTSTRAP_RELEASE}_${BOOTSTRAP_ARCH}") | ||
URL=$(echo $(eval echo "${key}")) |
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 see what you're trying to do here, but considering all the setup and checking necessary, is this really shorter than just using if statements to explicitly set the URL?
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 idea behind that is, that if we can't be sure the download links keep the same folder structure over time, we just add a new identifier for, let's say leap-42.3 and we are fine.
This means that we need to convert "leap-42.3" to "leap_42_3" which is done in a dump way up to now. For the next revision I will change this using sed. So in effect if a new Leap release is added, we just need to put url_leap_42_3_aarch64 URL in the file and we are settled. This is still worse then other distributions in crouton, but IMHO better then start adding if statements of every new release. Thoughts?
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.
A new if statement (or a case statement) would probably be easier to follow, but maybe there's some complexity in that approach I'm not seeing.
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 think with this approach, adding a new version/architecture is easier, as it is a one-line add.
When you put that in if statements, it's more complicated, as you have to figure out between different if's, the arch and distribution name.
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.
How about building up the URL in pieces?
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.
Ok, I hacked that up for you, let me know if you prefer this, I definitely do not. File path and file names are not homogenous on architectures and releases. I expect this to break with a new architecture. Apart I'm quite sure that if you don't have the actual links in front of you, it is really hard to understand how this links will look like:
BOOTSTRAP_ARCH="$ARCH"
BOOTSTRAP_RELEASE="$RELEASE"
CURRENT=""
if [ "$ARCH" = "arm64" ] || [ "$ARCH" = "aarch64" ]; then
BOOTSTRAP_ARCH="aarch64"
BOOTSTRAP_ARCH2="aarch64"
BOOTSTRAP_ARCH3="aarch64"
elif [ "$ARCH" = "armv7hl" ]; then
BOOTSTRAP_ARCH="armv7hl"
BOOTSTRAP_ARCH2="armv7hl"
BOOTSTRAP_ARCH3="armv7l"
fi
if [ "$RELEASE" = "factory" ] || [ "$RELEASE" = "tumbleweed" ]; then
BOOTSTRAP_VERSION="."
BOOTSTRAP_RELEASE="tumbleweed/images"
BOOTSTRAP_RELEASE2="."
BOOTSTRAP_RELEASE3="Tumbleweed"
CURRENT="-Current"
fi
case "$RELEASE" in leap*)
BOOTSTRAP_VERSION=${RELEASE#leap-}
BOOTSTRAP_RELEASE="distribution/leap"
BOOTSTRAP_RELEASE2="appliances"
BOOTSTRAP_RELEASE3="Leap$BOOTSTRAP_VERSION"
if [ "$ARCH" = "armv7hl" ]; then
BOOTSTRAP_ARCH2="armv7"
fi
esac
URL="http://download.opensuse.org/ports/$BOOTSTRAP_ARCH/$BOOTSTRAP_RELEASE/$BOOTSTRAP_VERSION/$BOOTSTRAP_RELEASE2/openSUSE-$BOOTSTRAP_RELEASE3-ARM-JeOS.$BOOTSTRAP_ARCH2-rootfs.$BOOTSTRAP_ARCH3$CURRENT.tbz"
So for the record, here are the links we are talking about up to now:
http://download.opensuse.org/ports/armv7hl/tumbleweed/images/openSUSE-Tumbleweed-ARM-JeOS.armv7hl-rootfs.armv7l-Current.tbz
http://download.opensuse.org/ports/aarch64/tumbleweed/images/openSUSE-Tumbleweed-ARM-JeOS.aarch64-rootfs.aarch64-Current.tbz
http://download.opensuse.org/ports/armv7hl/distribution/leap/42.2/appliances/openSUSE-Leap42.2-ARM-JeOS.armv7-rootfs.armv7l.tbz
http://download.opensuse.org/ports/aarch64/distribution/leap/42.2/appliances/openSUSE-Leap42.2-ARM-JeOS.aarch64-rootfs.aarch64.tbz
http://download.opensuse.org/ports/aarch64/distribution/leap/42.3/appliances/openSUSE-Leap42.3-ARM-JeOS.aarch64-rootfs.aarch64.tbz
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.
Haha alright, I'm convinced. Apologies for making you go through that.
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.
No problem, the important thing is, that we reached consens on this :)
installer/opensuse/bootstrap
Outdated
exit 1 | ||
fi | ||
|
||
mkdir -p $tmp/$subdir |
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.
quoting
installer/opensuse/bootstrap
Outdated
fi | ||
|
||
mkdir -p $tmp/$subdir | ||
( cd $tmp/$subdir; wget -O - $URL | bzip2 -d | tar x ) 1>&2 |
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.
quoting. Also, bzip2 -d | tar x
could just be tar -jx
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.
wget is being removed from the rootfs, so use curl:
curl -# -L --connect-timeout 60 --retry 2 "$URL" | tar -jx -C "$tmp/$subdir"
installer/opensuse/bootstrap
Outdated
mkdir -p $tmp/$subdir | ||
( cd $tmp/$subdir; wget -O - $URL | bzip2 -d | tar x ) 1>&2 | ||
|
||
echo 'Remove /dev files as not needed' |
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 don't think the user needs to see this
installer/opensuse/bootstrap
Outdated
( cd $tmp/$subdir; wget -O - $URL | bzip2 -d | tar x ) 1>&2 | ||
|
||
echo 'Remove /dev files as not needed' | ||
rm -rf ${tmp}/${subdir}/dev/* |
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.
quoting. you also don't need braces around the variable names
installer/opensuse/defaults
Outdated
# It must set at least ARCH and MIRROR if not already specified. | ||
|
||
if [ -z "$ARCH" ]; then | ||
ARCH="`uname -m`" |
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.
use $( )
installer/opensuse/getrelease.sh
Outdated
|
||
# Create release name from /etc/os-release file | ||
# sed specialist can make that more elegant... | ||
rel=$(grep CPE_NAME "$sources" | cut -d ":" -f 4) |
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.
quoting
installer/opensuse/getrelease.sh
Outdated
# Create release name from /etc/os-release file | ||
# sed specialist can make that more elegant... | ||
rel=$(grep CPE_NAME "$sources" | cut -d ":" -f 4) | ||
if [ $rel = "opensuse" ]; then |
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.
quoting...
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.
going to stop pointing it out here...but you need to make sure all your variable expansions are properly quoted
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.
ok, hopefully I will catch all of them.
installer/opensuse/prepare
Outdated
pkgsasdeps="`list_uninstalled_dist '' $pkgs`" | ||
fi | ||
zypper -q install -y $params $pkgs `list_uninstalled_dist - "$@"` || return $? | ||
# TODO see how we can implement that reasonable with zypper |
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.
what's the plan here?
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.
There is none such functionality in zypper, I think we should get rid of all the asdeps stuff in this file. Thoughts?
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.
Sure; the resulting chroot will just be larger because of it.
installer/opensuse/prepare
Outdated
fi | ||
shift | ||
done | ||
while [ ! "$#" = 0 ]; do |
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.
!=
installer/opensuse/prepare
Outdated
params='--no-recommends' | ||
shift | ||
fi | ||
if [ ! "$#" = 0 ]; then |
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.
!=
installer/opensuse/prepare
Outdated
|
||
%files | ||
EOF | ||
rpmbuild --define='_rpmdir ${tmp}' -bb ${tmp}/dummy.spec > ${tmp}/build.log |
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.
quoting. also, is single quotes correct for --define?
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.
yes, man page shows single quotes as well.
installer/opensuse/prepare
Outdated
%files | ||
EOF | ||
rpmbuild --define='_rpmdir ${tmp}' -bb ${tmp}/dummy.spec > ${tmp}/build.log | ||
if [ $? != 0 ] |
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.
if [ ]; then
installer/opensuse/prepare
Outdated
done | ||
} | ||
|
||
if [ -r /debootstrap.sh ]; then |
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.
not needed?
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.
no, not needed.
installer/opensuse/prepare
Outdated
chmod u+s /usr/bin/ping | ||
|
||
# add rpmbuild for dummy packages | ||
sudo zypper -q install -y --no-recommends rpmbuild |
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.
no need for sudo. also, shouldn't this be an install rpmbuild
command in core?
targets/chromium
Outdated
[ "${ARCH#arm}" != "$ARCH" ]; then | ||
error 99 "chromium target is not supported on Debian/ARM." | ||
error 99 "chromium target is not supported on "$DISTRO"/"$ARCH"." |
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.
this quoting isn't quite what you want
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.
is it really not available though? seems like it exists, although I have no idea what it involves: https://software.opensuse.org/package/chromium
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.
It's only available for x86, not for ARM.
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.
would something like this be acceptable for you:
error 99 "chromium target is not supported on "$DISTRO"/ARM."
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.
you don't need the quotes around $DISTRO, since the entire string is quoted
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.
and I'd prefer "chromium target is not supported on $DISTRO on ARM"
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.
sure, no problem
targets/x11-common
Outdated
@@ -7,6 +7,12 @@ | |||
# forms of X11. | |||
|
|||
### Append to prepare.sh: | |||
|
|||
#we need to install xinit to be sure we overwrite the path |
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.
what does this mean
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.
TBH I can't remember, but we need to install xinit, otherwise graphic does not work.
Do you prefer that I add this line instead of the if:
install opensuse=xinit,
Just let me know.
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'd prefer to not install things unnecessarily, so please check what xinit is pulling in when you try to install it here
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.
It's not pulling in anything, no recommends and no sugguests:
linux@localhost:~> zypper info --recommends --suggests xinit
Loading repository data...
Reading installed packages...
Information for package xinit:
------------------------------
Repository : openSUSE-Ports-Leap-42.3-repo-oss
Name : xinit
Version : 1.3.4-3.1
Arch : aarch64
Vendor : openSUSE
Installed Size : 234.6 KiB
Installed : Yes (automatically)
Status : up-to-date
Source package : xinit-1.3.4-3.1.src
Summary : X Window System initializer
Description :
The xinit program is used to start the X Window System server and a
first client program on systems that are not using a display manager
such as xdm or in environments that use multiple window systems.
When this first client exits, xinit will kill the X server and then
terminate.
Recommends : ---
Suggests : ---
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.
Ah, I see. This is fine, then. Just move it under the "remove old vgem hack" section and quote $DISTROAKA
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.
Oh weird, we're already installing xinit in the xorg. Is this an issue for xorg or only for xiwi?
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.
...in which case, xiwi line 31 is probably the right place for 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.
ok, I put it at line 31 in xiwi.
@dnschneid , I think there was an overlap between you adding more comments on the code and me updating the pull request. I integrated your last two comments. Please feel free to review :) |
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.
Lots of nits and a few minor errors, but no major concerns! Woo!
installer/opensuse/bootstrap
Outdated
fi | ||
|
||
# Fix leap release name | ||
case "$RELEASE" in leap*) |
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.
elif [ "${RELEASE#leap}" != "$RELEASE" ]; then
installer/opensuse/bootstrap
Outdated
|
||
# Fix leap release name | ||
case "$RELEASE" in leap*) | ||
BOOTSTRAP_RELEASE=$(echo "$RELEASE" | sed -e 's/[-.]/_/g') |
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.
quoting, and tr would probably be a better choice
installer/opensuse/bootstrap
Outdated
eval "URL=\"\${url_${BOOTSTRAP_RELEASE}_${BOOTSTRAP_ARCH}-}\"" | ||
|
||
if [ -z "$URL" ]; then | ||
echo "Unknown Distribution / Architecture: $key" |
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.
if the error
function is available, you probably want to use that. Otherwise, you want to redirect to STDERR using >&2
Also, $key
doesn't exist anymore
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.
it seems, that no error function is available.
installer/opensuse/bootstrap
Outdated
exit 1 | ||
fi | ||
|
||
mkdir -p "$tmp"/"$subdir" |
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.
"$tmp/$subdir"
installer/opensuse/bootstrap
Outdated
fi | ||
|
||
mkdir -p "$tmp"/"$subdir" | ||
( cd "$tmp"/"$subdir"; wget -O - $URL | tar -jx ) 1>&2 |
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.
wget is being removed from the rootfs, so use curl:
curl -# -L --connect-timeout 60 --retry 2 "$URL" | tar -jx -C "$tmp/$subdir"
targets/audio
Outdated
@@ -99,6 +99,13 @@ build_cras() { | |||
ln -sfT libasound.so.2 "$libasoundso" | |||
fi | |||
ALSALIBDIR="/usr/lib$archextrapath/alsa-lib" | |||
elif [ "$DISTROAKA" = "opensuse" ]; then | |||
install --minimal --asdeps alsa-devel | |||
if [ "$ARCH" = "arm64" ] || [ "$ARCH" = "x86_64"]; then |
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.
] || [
should be -o
targets/chromium
Outdated
@@ -3,9 +3,9 @@ | |||
# Use of this source code is governed by a BSD-style license that can be | |||
# found in the LICENSE file. | |||
if [ "${TARGETNOINSTALL:-c}" = 'c' ] && | |||
[ "$DISTRO" = 'debian' -o "$DISTRO" = 'kali' ] && | |||
[ "$DISTRO" = 'debian' -o "$DISTRO" = 'kali' -o "$DISTRO" = 'opensuse' ] && |
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.
not distroaka?
targets/gtk-extra
Outdated
@@ -8,17 +8,19 @@ DESCRIPTION='GTK-based tools including gdebi, gksu, and a simple browser.' | |||
|
|||
### Append to prepare.sh: | |||
install_dummy network-manager network-manager-gnome | |||
install gdebi gksu | |||
install opensuse=,gdebi opensuse=libgnomesu,gksu |
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.
indentation
targets/xiwi
Outdated
|
||
# We need to install xinit to make graphics work | ||
if [ $DISTROAKA = "opensuse" ]; then | ||
install xinit |
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.
Just add it as opensuse=xinit,
in the install line below
targets/xorg
Outdated
x11-xserver-utils xauth xinit xfonts-utils xkb-data xorg-docs-core \ | ||
xterm x11-common xinput | ||
else | ||
install xorg $installvideodrivers -- xserver-xorg-video-all$backport \ | ||
install opensuse=xorg-x11-server,xorg $installvideodrivers -- opensuse=xorg-x11-driver-video,xserver-xorg-video-all$backport \ |
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.
probably need to break the line
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Signed-off-by: Matthias Brugger <[email protected]>
Hi @dnschneid just now the newest version. Feel free to comment :) |
Gentle ping, would be nice if you find a moment to review :-) |
any further comments on that? |
if [ "$RELEASE" = "factory" ]; then | ||
BOOTSTRAP_RELEASE=tumbleweed | ||
elif [ "${RELEASE#leap}" != "$RELEASE" ]; then | ||
BOOTSTRAP_RELEASE=$(echo "$RELEASE" | tr - _ | tr . _) |
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.
quoting (and line below)
case "$ARCH" in | ||
arm*) ARCH="armv7hl";; | ||
arm64 | aarch64) ARCH="arm64";; | ||
*) error 2 "Invalid architecture '$ARCH'.";; |
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.
definitely need x86 support before we can merge; this would confuse most of our users
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.
Sorry I'm so slow at looking at this, especially since you've done such a great job cleaning it up.
It would be helpful if you avoid squashing fixes back into old commits (maybe until it's approved for merge and then you can rebase and make it pretty), since that causes the Github review UI to drop most of the history and comments and results in me needing to do a full review every time you update.
rpmbuild --define='_rpmdir "$tmp"' -bb "$tmp/dummy.spec" > "$tmp/build.log" | ||
if [ $? != 0 ]; then | ||
echo "ERROR: Could not build dummy rpm!" | ||
exit 1 |
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.
spaces, not tabs
455c029
to
cebf84f
Compare
Know issues:
normal instalation should work without problems:
sudo sh crouton -r tumbleweed -n test -t xiwi,xfce,keyboard,cli-extra
sudo sh crouton -r leap-42.2 -n test -t xiwi,xfce,keyboard,cli-extra